Re: [Qemu-devel] [PATCH v4 2/3] Extract code to nbd_setup function to be used for many purposes

[Chunyan Liu]

Thanks for your suggestions. Revision.

Add -f option to qemu-nbd.

Signed-off-by: Chunyan Liu <address@hidden>
---
 qemu-nbd.c |   76 ++++++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 291cba2..2dc0742 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"

 #define NBD_BUFFER_SIZE (1024*1024)

@@ -53,12 +53,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"
@@ -67,7 +68,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;
@@ -215,9 +217,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 */
@@ -256,7 +291,7 @@ int main(int argc, char **argv)
     struct sockaddr_in addr;
     socklen_t addr_len = sizeof(addr);
     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' },
@@ -273,6 +308,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;
@@ -291,6 +327,7 @@ int main(int argc, char **argv)
     int nb_fds = 0;
     int max_fd;
     int persistent = 0;
+    int find = 0;
     pthread_t client_thread;

     /* The client thread uses SIGTERM to interrupt the server.  A signal
@@ -374,6 +411,9 @@ int main(int argc, char **argv)
         case 'v':
             verbose = 1;
             break;
+        case 'f':
+            find = 1;
+            break;
         case 'V':
             version(argv[0]);
             exit(0);
@@ -408,7 +448,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;
@@ -460,18 +500,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());
         }
     }

@@ -503,10 +535,10 @@ int main(int argc, char **argv)
     if (sharing_fds[0] == -1)
         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));
@@ -574,7 +606,7 @@ int main(int argc, char **argv)
         unlink(sockpath);
     }

-    if (device) {
+    if (device || find) {
         void *ret;
         pthread_join(client_thread, &ret);
         exit(ret != NULL);
-- 
1.7.3.4



2011/12/8 Stefan Hajnoczi <address@hidden>:
> On Wed, Dec 7, 2011 at 4:23 AM, Chunyan Liu <address@hidden> wrote:
>
> Overall looks good, some suggestions:
>
>> @@ -53,7 +53,7 @@ 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-%s)\n"
>>  "  -r, --read-only      export read-only\n"
>>  "  -P, --partition=NUM  only expose partition NUM\n"
>>  "  -s, --snapshot       use snapshot file\n"
>> @@ -67,7 +67,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, "PID");
>>  }
>
> Since we're not using the SOCKET_PATH format string anymore it's nicer
> to drop this format string argument and just change to "(default
> /var/lock/qemu-nbd-PID" above.
>
>> +        fd = open(device, O_RDWR);
>> +        if (fd == -1) {
>> +            fprintf(stderr, "Failed to open %s", device);
>
> Missing \n in string.
>
>> +            goto out;
>> +        }
>> +
>> +        ret = nbd_init(fd, sock, nbdflags, size, blocksize);
>> +        if (ret == -1) {
>> +            goto out;
>> +        }
>> +    } else {
>> +        int i = 0;
>> +        int max_nbd = 16;
>> +        for (i = 0; i < max_nbd; i++) {
>> +            if (!asprintf(&device, "/dev/nbd%d", i)) {
>
> asprintf() is GNU and BSD only (not C or POSIX).  I suggest using
> g_strdup_printf() instead which is guaranteed to be available because
> QEMU builds against glib:

Using g_malloc, snprintf, g_free instead. Doesn't need extra header file.

> http://developer.gnome.org/glib/2.28/glib-String-Utility-Functions.html#g-strdup-printf
>
>> +                continue;
>> +            }
>>
>> +
>> +            fd = open(device, O_RDWR);
>> +            if (fd == -1 || nbd_init(fd, sock, nbdflags, size, blocksize)
>> == -1) {
>
> nbd_init() does not close(fd) on failure.  Please separate the open()
> and nbd_init() error cases and close the fd.
>
> Stefan
>