Re: [Qemu-devel] [PATCH][resend] Add -f option to qemu-nbd

[Paolo Bonzini <pbonzini@redhat.com>]

On 01/18/2012 09:48 AM, Chunyan Liu wrote:
> Stefan, could you help commit it if it's OK? Thanks.
> Same as in thread:
> http://lists.gnu.org/archive/html/qemu-devel/2011-12/msg01083.html
> but rebase it to latest code.
>
> Add -f option to qemu-nbd to find a free nbd device for user and connect
> disk image to that device.

Hi Chunyan,

I'm now the NBD maintainer so I can take care of the patch.  There are 
still a couple of nits.

The main problem is that the device name is never printed, which 
requires some imagination on part of the user. :)

This pretty much requires that you set "verbose = 1" together with -f. 
This disables daemonization, unfortunately, but there isn't really any 
better way to do so.  I'm inclined towards removing daemonization 
altogether from 1.1; other opinions are welcome.

Anyhow, please set "verbose = 1" and change the help text for -c/-v from

"  -c, --connect=DEV    connect FILE to the local NBD device DEV\n"

"  -v, --verbose        display extra debugging information\n"

to

"  -c, --connect=DEV    connect FILE to the local NBD device DEV\n"
"                       and run in the background\n"

"  -v, --verbose        with -c, display extra information and\n"
"                       do not run in the background\n"

Since a respin is required, there are a couple of other comments inline.

> syntax: qemu-nbd -f disk.img
> ---
>   qemu-nbd.c |   76 ++++++++++++++++++++++++++++++++++++++++++-----------------
>   1 files changed, 54 insertions(+), 22 deletions(-)
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index eb61c33..f4c1437 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -33,7 +33,7 @@
>   #include<libgen.h>
>   #include<pthread.h>
>
> -#define SOCKET_PATH    "/var/lock/qemu-nbd-%s"
> +#define SOCKET_PATH    "/var/lock/qemu-nbd-%d"
>
>   static NBDExport *exp;
>   static int verbose;
> @@ -55,12 +55,13 @@ static void usage(const char *name)
>   "  -o, --offset=OFFSET  offset into the image\n"
>   "  -b, --bind=IFACE     interface to bind to (default `0.0.0.0')\n"
>   "  -k, --socket=PATH    path to the unix socket\n"
> -"                       (default '"SOCKET_PATH"')\n"
> +"                       (default /var/lock/qemu-nbd-PID)\n"
>   "  -r, --read-only      export read-only\n"
>   "  -P, --partition=NUM  only expose partition NUM\n"
>   "  -s, --snapshot       use snapshot file\n"
>   "  -n, --nocache        disable host cache\n"
>   "  -c, --connect=DEV    connect FILE to the local NBD device DEV\n"
> +"  -f, --find           find a free NBD device and connect FILE to it\n"
>   "  -d, --disconnect     disconnect the specified device\n"
>   "  -e, --shared=NUM     device can be shared by NUM clients (default '1')\n"
>   "  -t, --persistent     don't exit on the last connection\n"
> @@ -69,7 +70,7 @@ static void usage(const char *name)
>   "  -V, --version        output version information and exit\n"
>   "\n"
>   "Report bugs to<anthony@codemonkey.ws>\n"
> -    , name, NBD_DEFAULT_PORT, "DEVICE");
> +    , name, NBD_DEFAULT_PORT);
>   }
>
>   static void version(const char *name)
> @@ -194,7 +195,8 @@ static void *show_parts(void *arg)
>
>   static void *nbd_client_thread(void *arg)
>   {
> -    int fd = *(int *)arg;
> +    int fd = -1;
> +    int find = *(int *)arg;
>       off_t size;
>       size_t blocksize;
>       uint32_t nbdflags;
> @@ -213,9 +215,42 @@ static void *nbd_client_thread(void *arg)
>           goto out;
>       }
>
> -    ret = nbd_init(fd, sock, nbdflags, size, blocksize);
> -    if (ret == -1) {
> -        goto out;
> +    if (!find) {
> +        fd = open(device, O_RDWR);
> +        if (fd == -1) {
> +            fprintf(stderr, "Failed to open %s\n", device);
> +            goto out;
> +        }
> +
> +        ret = nbd_init(fd, sock, nbdflags, size, blocksize);
> +        if (ret == -1) {
> +            goto out;
> +        }
> +    } else {
> +        int i = 0;
> +        int max_nbd = 16;
> +        device = g_malloc(64);
> +
> +        for (i = 0; i<  max_nbd; i++) {
> +            snprintf(device, 64, "/dev/nbd%d", i);
> +            fd = open(device, O_RDWR);
> +            if (fd == -1) {
> +                continue;
> +            }
> +
> +            if (nbd_init(fd, sock, nbdflags, size, blocksize) == -1) {
> +                close(fd);
> +                continue;
> +            }
> +
> +            break;
> +        }
> +
> +        if (i>= max_nbd) {
> +            fprintf(stderr, "Fail to find a free nbd device\n");
> +            g_free(device);
> +            goto out;

Just use errx(EXIT_FAILURE, "Could not find a free NBD device.") here.

> +        }
>       }
>
>       /* update partition table */
> @@ -275,7 +310,7 @@ int main(int argc, char **argv)
>       const char *bindto = "0.0.0.0";
>       int port = NBD_DEFAULT_PORT;
>       off_t fd_size;
> -    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:t";
> +    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:tf";
>       struct option lopt[] = {
>           { "help", 0, NULL, 'h' },
>           { "version", 0, NULL, 'V' },
> @@ -292,6 +327,7 @@ int main(int argc, char **argv)
>           { "shared", 1, NULL, 'e' },
>           { "persistent", 0, NULL, 't' },
>           { "verbose", 0, NULL, 'v' },
> +        { "find", 0, NULL, 'f'},
>           { NULL, 0, NULL, 0 }
>       };
>       int ch;
> @@ -303,6 +339,7 @@ int main(int argc, char **argv)
>       int ret;
>       int fd;
>       int persistent = 0;
> +    int find = 0;
>       pthread_t client_thread;
>
>       /* The client thread uses SIGTERM to interrupt the server.  A signal
> @@ -380,6 +417,9 @@ int main(int argc, char **argv)
>           case 'v':
>               verbose = 1;
>               break;
> +        case 'f':
> +            find = 1;
> +            break;
>           case 'V':
>               version(argv[0]);
>               exit(0);
> @@ -414,7 +454,7 @@ int main(int argc, char **argv)
>   	return 0;
>       }

Please give an error here if "device && find" ("The -f option is 
incompatible with -c").

> -    if (device&&  !verbose) {
> +    if ((device || find)&&  !verbose) {
>           int stderr_fd[2];
>           pid_t pid;
>           int ret;
> @@ -466,18 +506,10 @@ int main(int argc, char **argv)
>           }
>       }
>
> -    if (device) {
> -        /* Open before spawning new threads.  In the future, we may
> -         * drop privileges after opening.
> -         */
> -        fd = open(device, O_RDWR);
> -        if (fd == -1) {
> -            err(EXIT_FAILURE, "Failed to open %s", device);
> -        }
> -
> +    if (device || find) {
>           if (sockpath == NULL) {
>               sockpath = g_malloc(128);
> -            snprintf(sockpath, 128, SOCKET_PATH, basename(device));
> +            snprintf(sockpath, 128, SOCKET_PATH, getpid());
>           }
>       }
>
> @@ -510,10 +542,10 @@ int main(int argc, char **argv)
>           return 1;
>       }
>
> -    if (device) {
> +    if (device || find) {
>           int ret;
>
> -        ret = pthread_create(&client_thread, NULL, nbd_client_thread,&fd);
> +        ret = pthread_create(&client_thread, NULL, nbd_client_thread,&find);

Just pass NULL here and test "device != NULL" in the nbd_client_thread.

>           if (ret != 0) {
>               errx(EXIT_FAILURE, "Failed to create client thread: %s",
>                    strerror(ret));
> @@ -536,7 +568,7 @@ int main(int argc, char **argv)
>           unlink(sockpath);
>       }
>
> -    if (device) {
> +    if (device || find) {
>           void *ret;
>           pthread_join(client_thread,&ret);
>           exit(ret != NULL);

Thanks,

Paolo