| Message ID | 20181215135324.152629-7-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nbd: add qemu-nbd --list | expand |
On Sat, Dec 15, 2018 at 07:53:08AM -0600, Eric Blake wrote: > Connecting to a /dev/nbdN device is a Linux-specific action. > We were already masking -c and -d from 'qemu-nbd --help' on > non-linux. However, while -d fails with a sensible error > message, it took hunting through a couple of files to prove > that. What's more, the code for -c doesn't fail until after > it has created a pthread and tried to open a device - possibly > even printing an error message with %m on a non-Linux platform > in spite of the comment that %m is glibc-specific. Make the > failure happen sooner, then get rid of stubs that are no > longer needed because of the early exits. > > While at it: tweak the blank newlines in --help output to be > consistent, whether or not built on Linux. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v2: Hoist -c error message to share with -d message [Vladimir] > Bonus: gets rid of a stray TAB in nbd/client.c > --- > nbd/client.c | 18 +----------------- > qemu-nbd.c | 21 +++++++++++++++++++-- > 2 files changed, 20 insertions(+), 19 deletions(-) > > diff --git a/nbd/client.c b/nbd/client.c > index 5d59d5ba78a..3d9086af39d 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -1029,23 +1029,7 @@ int nbd_disconnect(int fd) > return 0; > } > > -#else > -int nbd_init(int fd, QIOChannelSocket *ioc, NBDExportInfo *info, > - Error **errp) > -{ > - error_setg(errp, "nbd_init is only supported on Linux"); > - return -ENOTSUP; > -} > - > -int nbd_client(int fd) > -{ > - return -ENOTSUP; > -} > -int nbd_disconnect(int fd) > -{ > - return -ENOTSUP; > -} > -#endif > +#endif /* __linux__ */ > > int nbd_send_request(QIOChannel *ioc, NBDRequest *request) > { > diff --git a/qemu-nbd.c b/qemu-nbd.c > index e169b839ece..2807e132396 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -43,6 +43,12 @@ > #include "trace/control.h" > #include "qemu-version.h" > > +#ifdef __linux__ > +#define HAVE_NBD_DEVICE 1 > +#else > +#define HAVE_NBD_DEVICE 0 > +#endif > + > #define SOCKET_PATH "/var/lock/qemu-nbd-%s" > #define QEMU_NBD_OPT_CACHE 256 > #define QEMU_NBD_OPT_AIO 257 > @@ -98,11 +104,11 @@ static void usage(const char *name) > " specify tracing options\n" > " --fork fork off the server process and exit the parent\n" > " once the server is running\n" > -#ifdef __linux__ > +#if HAVE_NBD_DEVICE > +"\n" > "Kernel NBD client support:\n" > " -c, --connect=DEV connect FILE to the local NBD device DEV\n" > " -d, --disconnect disconnect the specified device\n" > -"\n" > #endif > "\n" > "Block device options:\n" > @@ -236,6 +242,7 @@ static void termsig_handler(int signum) > } > > > +#if HAVE_NBD_DEVICE > static void *show_parts(void *arg) > { > char *device = arg; > @@ -321,6 +328,7 @@ out: > kill(getpid(), SIGTERM); > return (void *) EXIT_FAILURE; > } > +#endif /* HAVE_NBD_DEVICE */ > > static int nbd_can_accept(void) > { > @@ -814,6 +822,12 @@ int main(int argc, char **argv) > } > } > > +#if !HAVE_NBD_DEVICE > + if (disconnect || device) { > + error_report("Kernel /dev/nbdN support not available"); > + exit(EXIT_FAILURE); > + } > +#else /* HAVE_NBD_DEVICE */ > if (disconnect) { > int nbdfd = open(argv[optind], O_RDWR); > if (nbdfd < 0) { > @@ -829,6 +843,7 @@ int main(int argc, char **argv) > > return 0; > } > +#endif > > if ((device && !verbose) || fork_process) { > int stderr_fd[2]; > @@ -1006,6 +1021,7 @@ int main(int argc, char **argv) > nbd_export_set_description(exp, export_description); > > if (device) { > +#if HAVE_NBD_DEVICE > int ret; > > ret = pthread_create(&client_thread, NULL, nbd_client_thread, device); > @@ -1013,6 +1029,7 @@ int main(int argc, char **argv) > error_report("Failed to create client thread: %s", strerror(ret)); > exit(EXIT_FAILURE); > } > +#endif > } else { > /* Shut up GCC warnings. */ > memset(&client_thread, 0, sizeof(client_thread)); > -- An obvious simplification of the last version, so: Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich.
15.12.2018 16:53, Eric Blake wrote: > Connecting to a /dev/nbdN device is a Linux-specific action. > We were already masking -c and -d from 'qemu-nbd --help' on > non-linux. However, while -d fails with a sensible error > message, it took hunting through a couple of files to prove > that. What's more, the code for -c doesn't fail until after > it has created a pthread and tried to open a device - possibly > even printing an error message with %m on a non-Linux platform > in spite of the comment that %m is glibc-specific. Make the > failure happen sooner, then get rid of stubs that are no > longer needed because of the early exits. > > While at it: tweak the blank newlines in --help output to be > consistent, whether or not built on Linux. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v2: Hoist -c error message to share with -d message [Vladimir] > Bonus: gets rid of a stray TAB in nbd/client.c > --- > nbd/client.c | 18 +----------------- > qemu-nbd.c | 21 +++++++++++++++++++-- > 2 files changed, 20 insertions(+), 19 deletions(-) > > diff --git a/nbd/client.c b/nbd/client.c > index 5d59d5ba78a..3d9086af39d 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -1029,23 +1029,7 @@ int nbd_disconnect(int fd) > return 0; > } > > -#else > -int nbd_init(int fd, QIOChannelSocket *ioc, NBDExportInfo *info, > - Error **errp) > -{ > - error_setg(errp, "nbd_init is only supported on Linux"); > - return -ENOTSUP; > -} > - > -int nbd_client(int fd) > -{ > - return -ENOTSUP; > -} > -int nbd_disconnect(int fd) > -{ > - return -ENOTSUP; > -} > -#endif > +#endif /* __linux__ */ > > int nbd_send_request(QIOChannel *ioc, NBDRequest *request) > { > diff --git a/qemu-nbd.c b/qemu-nbd.c > index e169b839ece..2807e132396 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -43,6 +43,12 @@ > #include "trace/control.h" > #include "qemu-version.h" > > +#ifdef __linux__ > +#define HAVE_NBD_DEVICE 1 > +#else > +#define HAVE_NBD_DEVICE 0 > +#endif > + > #define SOCKET_PATH "/var/lock/qemu-nbd-%s" > #define QEMU_NBD_OPT_CACHE 256 > #define QEMU_NBD_OPT_AIO 257 > @@ -98,11 +104,11 @@ static void usage(const char *name) > " specify tracing options\n" > " --fork fork off the server process and exit the parent\n" > " once the server is running\n" > -#ifdef __linux__ > +#if HAVE_NBD_DEVICE > +"\n" > "Kernel NBD client support:\n" > " -c, --connect=DEV connect FILE to the local NBD device DEV\n" > " -d, --disconnect disconnect the specified device\n" > -"\n" > #endif > "\n" > "Block device options:\n" > @@ -236,6 +242,7 @@ static void termsig_handler(int signum) > } > > > +#if HAVE_NBD_DEVICE > static void *show_parts(void *arg) > { > char *device = arg; > @@ -321,6 +328,7 @@ out: > kill(getpid(), SIGTERM); > return (void *) EXIT_FAILURE; > } > +#endif /* HAVE_NBD_DEVICE */ > > static int nbd_can_accept(void) > { > @@ -814,6 +822,12 @@ int main(int argc, char **argv) > } > } > > +#if !HAVE_NBD_DEVICE > + if (disconnect || device) { > + error_report("Kernel /dev/nbdN support not available"); > + exit(EXIT_FAILURE); > + } > +#else /* HAVE_NBD_DEVICE */ hm, still there are two places (did I miss it during previous review?), where device variable is used, so it may be better to move this hunk even upper. On the other hand, one of these places is unreachable under windows and second is error path anyway, so, with, or without moving up: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > if (disconnect) { > int nbdfd = open(argv[optind], O_RDWR); > if (nbdfd < 0) { > @@ -829,6 +843,7 @@ int main(int argc, char **argv) > > return 0; > } > +#endif > > if ((device && !verbose) || fork_process) { > int stderr_fd[2]; > @@ -1006,6 +1021,7 @@ int main(int argc, char **argv) > nbd_export_set_description(exp, export_description); > > if (device) { aha, HAVE_NBD_DEVICE is definitely enabled here, we need #if only to mask symbol which is undefined if HAVE_NBD_DEVICE is 0. it may look funny, but why not add assert(HAVE_NBD_DEVICE); here ? :) > +#if HAVE_NBD_DEVICE > int ret; > > ret = pthread_create(&client_thread, NULL, nbd_client_thread, device); > @@ -1013,6 +1029,7 @@ int main(int argc, char **argv) > error_report("Failed to create client thread: %s", strerror(ret)); > exit(EXIT_FAILURE); > } > +#endif > } else { > /* Shut up GCC warnings. */ > memset(&client_thread, 0, sizeof(client_thread)); >
diff --git a/nbd/client.c b/nbd/client.c index 5d59d5ba78a..3d9086af39d 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -1029,23 +1029,7 @@ int nbd_disconnect(int fd) return 0; } -#else -int nbd_init(int fd, QIOChannelSocket *ioc, NBDExportInfo *info, - Error **errp) -{ - error_setg(errp, "nbd_init is only supported on Linux"); - return -ENOTSUP; -} - -int nbd_client(int fd) -{ - return -ENOTSUP; -} -int nbd_disconnect(int fd) -{ - return -ENOTSUP; -} -#endif +#endif /* __linux__ */ int nbd_send_request(QIOChannel *ioc, NBDRequest *request) { diff --git a/qemu-nbd.c b/qemu-nbd.c index e169b839ece..2807e132396 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -43,6 +43,12 @@ #include "trace/control.h" #include "qemu-version.h" +#ifdef __linux__ +#define HAVE_NBD_DEVICE 1 +#else +#define HAVE_NBD_DEVICE 0 +#endif + #define SOCKET_PATH "/var/lock/qemu-nbd-%s" #define QEMU_NBD_OPT_CACHE 256 #define QEMU_NBD_OPT_AIO 257 @@ -98,11 +104,11 @@ static void usage(const char *name) " specify tracing options\n" " --fork fork off the server process and exit the parent\n" " once the server is running\n" -#ifdef __linux__ +#if HAVE_NBD_DEVICE +"\n" "Kernel NBD client support:\n" " -c, --connect=DEV connect FILE to the local NBD device DEV\n" " -d, --disconnect disconnect the specified device\n" -"\n" #endif "\n" "Block device options:\n" @@ -236,6 +242,7 @@ static void termsig_handler(int signum) } +#if HAVE_NBD_DEVICE static void *show_parts(void *arg) { char *device = arg; @@ -321,6 +328,7 @@ out: kill(getpid(), SIGTERM); return (void *) EXIT_FAILURE; } +#endif /* HAVE_NBD_DEVICE */ static int nbd_can_accept(void) { @@ -814,6 +822,12 @@ int main(int argc, char **argv) } } +#if !HAVE_NBD_DEVICE + if (disconnect || device) { + error_report("Kernel /dev/nbdN support not available"); + exit(EXIT_FAILURE); + } +#else /* HAVE_NBD_DEVICE */ if (disconnect) { int nbdfd = open(argv[optind], O_RDWR); if (nbdfd < 0) { @@ -829,6 +843,7 @@ int main(int argc, char **argv) return 0; } +#endif if ((device && !verbose) || fork_process) { int stderr_fd[2]; @@ -1006,6 +1021,7 @@ int main(int argc, char **argv) nbd_export_set_description(exp, export_description); if (device) { +#if HAVE_NBD_DEVICE int ret; ret = pthread_create(&client_thread, NULL, nbd_client_thread, device); @@ -1013,6 +1029,7 @@ int main(int argc, char **argv) error_report("Failed to create client thread: %s", strerror(ret)); exit(EXIT_FAILURE); } +#endif } else { /* Shut up GCC warnings. */ memset(&client_thread, 0, sizeof(client_thread));
Connecting to a /dev/nbdN device is a Linux-specific action. We were already masking -c and -d from 'qemu-nbd --help' on non-linux. However, while -d fails with a sensible error message, it took hunting through a couple of files to prove that. What's more, the code for -c doesn't fail until after it has created a pthread and tried to open a device - possibly even printing an error message with %m on a non-Linux platform in spite of the comment that %m is glibc-specific. Make the failure happen sooner, then get rid of stubs that are no longer needed because of the early exits. While at it: tweak the blank newlines in --help output to be consistent, whether or not built on Linux. Signed-off-by: Eric Blake <eblake@redhat.com> --- v2: Hoist -c error message to share with -d message [Vladimir] Bonus: gets rid of a stray TAB in nbd/client.c --- nbd/client.c | 18 +----------------- qemu-nbd.c | 21 +++++++++++++++++++-- 2 files changed, 20 insertions(+), 19 deletions(-)