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

[Chunyan Liu]

Thanks, Paolo!

2012/1/18 Paolo Bonzini <address@hidden>:
> 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<address@hidden>\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
>