Patchwork [resend] Add -f option to qemu-nbd

login
register
mail settings
Submitter Chunyan Liu
Date Jan. 18, 2012, 8:48 a.m.
Message ID <1326876486-13995-1-git-send-email-cyliu@suse.com>
Download mbox | patch
Permalink /patch/136571/
State New
Headers show

Comments

Chunyan Liu - Jan. 18, 2012, 8:48 a.m.
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.
syntax: qemu-nbd -f disk.img
---
 qemu-nbd.c |   76 ++++++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 54 insertions(+), 22 deletions(-)
Paolo Bonzini - Jan. 18, 2012, 10:51 a.m.
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
Michael Tokarev - Jan. 18, 2012, 10:56 a.m.
On 18.01.2012 12:48, 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.

There's a (trivial) fix sent against qemu-nbd which will
make this patch to not apply again (the change of "fd"
variable in main() into devfd and sockfd).  I dunno if
it is applied to any branch or not.  Most active person
in this area appears to be Paolo Bonzini (Cc'd).

See comments inline below.

> Add -f option to qemu-nbd to find a free nbd device for user and connect
> disk image to that device.
> 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"

This is a semantic change.  Before, the socket was
named after the nbd device name, which, lacking the
-f option, was deterministic (given with -c option).
Now, since pid is random for the user, we don't have
a deterministic socket name anymore.  I don't think
that using pid here makes any sense at all, especially
having in mind the double-fork the daemon is doing
at start.  I think that we should continue using the
nbd device name here as before.

Note also that, while it is not currently supported
anyway, you're making this more difficult to change
the socket path by hardcoding it into two places.
(It may need change due to /var/lock => /run/lock
transition but old /var/lock/ will continue to work
anyway, since it gets converted to a symlink pointing
to /run/lock).

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

You also can use !device condition here instead of extra
variable: if device is set (non-NULL) use it, if it is not
set, find.  This way there's no need to pass any args to
this function at all.  But that's just my taste, nothing
more.

>      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;
> +            }

Here, I'm not sure it should ignore all possible errors.
How about, say, EMFILE, or ENOENT, or lots of other possible
error conditions which may be reported here instead of
EBUSY?  Not that it is very important again...

> +
> +            break;
> +        }
> +
> +        if (i >= max_nbd) {
> +            fprintf(stderr, "Fail to find a free nbd device\n");

in all other places in qemu-nbd.c error reporting is done
differently, namely, using err() or errx() routine (or
warn()).  The difference is important: err(3) also reports
strerror(errno) if errno is nonzero, so it will be clear
_which_ error happened.  I understand that usage of err(3)
here is a bit more fragile since it is not a main thread.
Besides, it is "Failed" not "Fail".

Thanks,

/mjt
Paolo Bonzini - Jan. 18, 2012, 11:26 a.m.
On 01/18/2012 11:56 AM, Michael Tokarev wrote:
> On 18.01.2012 12:48, 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.
>
> There's a (trivial) fix sent against qemu-nbd which will
> make this patch to not apply again (the change of "fd"
> variable in main() into devfd and sockfd).  I dunno if
> it is applied to any branch or not.  Most active person
> in this area appears to be Paolo Bonzini (Cc'd).

I've not yet prepared a public NBD tree, but I will soon and I will 
apply that patch (strictly speaking I had a comment, but I can fix that 
up myself).

>> @@ -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"
>
> This is a semantic change.  Before, the socket was
> named after the nbd device name, which, lacking the
> -f option, was deterministic (given with -c option).
> Now, since pid is random for the user, we don't have
> a deterministic socket name anymore.  I don't think
> that using pid here makes any sense at all, especially
> having in mind the double-fork the daemon is doing
> at start.  I think that we should continue using the
> nbd device name here as before.

That is not possible.  The socket is needed by nbd_init, and you cannot 
know the name of the socket until you know the device name.

The socket name is really an internal detail when using -c.

> Note also that, while it is not currently supported
> anyway, you're making this more difficult to change
> the socket path by hardcoding it into two places.

Yes, that's true.  He could have two definitions (SOCKET_PATH with %d 
and SOCKET_PATH_HELP without) so that the occurrences would stay close 
in the source code.

>>   "  -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;
>
> You also can use !device condition here instead of extra
> variable: if device is set (non-NULL) use it, if it is not
> set, find.  This way there's no need to pass any args to
> this function at all.  But that's just my taste, nothing
> more.
>
>>       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;
>> +            }
>
> Here, I'm not sure it should ignore all possible errors.
> How about, say, EMFILE, or ENOENT, or lots of other possible
> error conditions which may be reported here instead of
> EBUSY?  Not that it is very important again...

Yes, though actually ENOENT is an important special case that is worth 
reporting to the user.  It happens when the nbd module is not loaded.

>> +
>> +            break;
>> +        }
>> +
>> +        if (i>= max_nbd) {
>> +            fprintf(stderr, "Fail to find a free nbd device\n");
>
> in all other places in qemu-nbd.c error reporting is done
> differently, namely, using err() or errx() routine (or
> warn()).  The difference is important: err(3) also reports
> strerror(errno) if errno is nonzero, so it will be clear
> _which_ error happened.  I understand that usage of err(3)
> here is a bit more fragile since it is not a main thread.
> Besides, it is "Failed" not "Fail".

Indeed, I suggested using errx in my review but err is better because of 
ENOENT.

Paolo
Chunyan Liu - Jan. 19, 2012, 8:52 a.m.
Thanks, Paolo!

2012/1/18 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.

Currently, to print the connected device name, we can also change code a little
as following: ("-v" can still indicate working on background or not. )
@@ -256,10 +256,9 @@ static void *nbd_client_thread(void *arg)
     /* update partition table */
     pthread_create(&show_parts_thread, NULL, show_parts, NULL);

-    if (verbose) {
-        fprintf(stderr, "NBD device %s is now connected to %s\n",
+    fprintf(stderr, "NBD device %s is now connected to %s\n",
                 device, srcpath);
-    } else {
+    if (!verbose) {
         /* Close stderr so that the qemu-nbd process exits.  */
         dup2(STDOUT_FILENO, STDERR_FILENO);
     }

>
> 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.

Using errx() will skip kill(getpid(), SIGTERM) and exit directly;
I'm not sure if that will cause problem?

>
>> +        }
>>      }
>>
>>      /* 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
>
Paolo Bonzini - Jan. 19, 2012, 9:03 a.m.
On 01/19/2012 09:52 AM, Chunyan Liu wrote:
> Currently, to print the connected device name, we can also change code a little
> as following: ("-v" can still indicate working on background or not. )

Unfortunately not, that will cause the parent to exit with a status code 
of 1.

Paolo
Chunyan Liu - Jan. 19, 2012, 9:16 a.m.
2012/1/18 Paolo Bonzini <pbonzini@redhat.com>:
> On 01/18/2012 11:56 AM, Michael Tokarev wrote:
>>
>> On 18.01.2012 12:48, 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.
>>
>>
>> There's a (trivial) fix sent against qemu-nbd which will
>> make this patch to not apply again (the change of "fd"
>> variable in main() into devfd and sockfd).  I dunno if
>> it is applied to any branch or not.  Most active person
>> in this area appears to be Paolo Bonzini (Cc'd).
>
>
> I've not yet prepared a public NBD tree, but I will soon and I will apply
> that patch (strictly speaking I had a comment, but I can fix that up
> myself).
>
>
>>> @@ -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"
>>
>>
>> This is a semantic change.  Before, the socket was
>> named after the nbd device name, which, lacking the
>> -f option, was deterministic (given with -c option).
>> Now, since pid is random for the user, we don't have
>> a deterministic socket name anymore.  I don't think
>> that using pid here makes any sense at all, especially
>> having in mind the double-fork the daemon is doing
>> at start.  I think that we should continue using the
>> nbd device name here as before.
>
>
> That is not possible.  The socket is needed by nbd_init, and you cannot know
> the name of the socket until you know the device name.
>
> The socket name is really an internal detail when using -c.
>
>
>> Note also that, while it is not currently supported
>> anyway, you're making this more difficult to change
>> the socket path by hardcoding it into two places.
>
>
> Yes, that's true.  He could have two definitions (SOCKET_PATH with %d and
> SOCKET_PATH_HELP without) so that the occurrences would stay close in the
> source code.
Not clear how it will use SOCKET_PATH and SOCKET_PATH_HELP.
SOCKET_PATH_HELP stores the abstract string of sock path? (e.g.
/var/lock/qemu-nbd-xxx)
>
>
>>>  "  -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;
>>
>>
>> You also can use !device condition here instead of extra
>> variable: if device is set (non-NULL) use it, if it is not
>> set, find.  This way there's no need to pass any args to
>> this function at all.  But that's just my taste, nothing
>> more.
>>
>>>      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;
>>> +            }
>>
>>
>> Here, I'm not sure it should ignore all possible errors.
>> How about, say, EMFILE, or ENOENT, or lots of other possible
>> error conditions which may be reported here instead of
>> EBUSY?  Not that it is very important again...
>
>
> Yes, though actually ENOENT is an important special case that is worth
> reporting to the user.  It happens when the nbd module is not loaded.
If nbd module is not loaded, open device will fail, we can add error info:
            fd = open(device, O_RDWR);
            if (fd == -1) {
                fprintf(stderr, "Failed to open %s\n", device);
                continue;
            }
And in next place, if using err instead of fprintf, it will print ENOENT error
message. Is that enough?
>
>
>>> +
>>> +            break;
>>> +        }
>>> +
>>> +        if (i>= max_nbd) {
>>> +            fprintf(stderr, "Fail to find a free nbd device\n");
>>
>>
>> in all other places in qemu-nbd.c error reporting is done
>> differently, namely, using err() or errx() routine (or
>> warn()).  The difference is important: err(3) also reports
>> strerror(errno) if errno is nonzero, so it will be clear
>> _which_ error happened.  I understand that usage of err(3)
>> here is a bit more fragile since it is not a main thread.
>> Besides, it is "Failed" not "Fail".
>
>
> Indeed, I suggested using errx in my review but err is better because of
> ENOENT.
>
> Paolo
>
Paolo Bonzini - Jan. 19, 2012, 9:34 a.m.
On 01/19/2012 10:16 AM, Chunyan Liu wrote:
>> >  Yes, that's true.  He could have two definitions (SOCKET_PATH with %d and
>> >  SOCKET_PATH_HELP without) so that the occurrences would stay close in the
>> >  source code.
> Not clear how it will use SOCKET_PATH and SOCKET_PATH_HELP.
> SOCKET_PATH_HELP stores the abstract string of sock path? (e.g.
> /var/lock/qemu-nbd-xxx)

Yes (s/xxx/PID/).

>> Yes, though actually ENOENT is an important special case that is worth
>> reporting to the user.  It happens when the nbd module is not loaded.
> If nbd module is not loaded, open device will fail, we can add error info:
>             fd = open(device, O_RDWR);
>             if (fd == -1) {
>                 fprintf(stderr, "Failed to open %s\n", device);
>                 continue;
>             }
> And in next place, if using err instead of fprintf, it will print ENOENT error
> message. Is that enough?

You do not need this fprintf.  Otherwise, yes.

Please build your patch on top of git://github.com/bonzini/qemu.git, 
branch nbd-next.

Thanks,

Paolo

Patch

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;
+        }
     }
 
     /* 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;
     }
 
-    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);
         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);