Re: [Qemu-devel] [PATCH v2 06/22] qemu-nbd: Fail earlier for -c/-d on non-linux

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
> 
> ---
> 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 <address@hidden>

>       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));
> 


-- 
Best regards,
Vladimir