Patchwork [v4,2/3] Extract code to nbd_setup function to be used for many purposes

login
register
mail settings
Submitter Chunyan Liu
Date Dec. 7, 2011, 4:23 a.m.
Message ID <CAERYnoaz3iUgA=TWuTaDWvovCiDpwgJst_5UjUDsg9z+8_z0tA@mail.gmail.com>
Download mbox | patch
Permalink /patch/129891/
State New
Headers show

Comments

Chunyan Liu - Dec. 7, 2011, 4:23 a.m.
Add -f option to qemu-nbd. New implementation. Do not need nbd_setup.
Tested and worked.

Signed-off-by: Chunyan Liu <cyliu@suse.com>
---
 qemu-nbd.c |   71
+++++++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 49 insertions(+), 22 deletions(-)

     /* update partition table */
@@ -256,7 +286,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 +303,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 +322,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 +406,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 +443,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 +495,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 +530,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 +601,7 @@ int main(int argc, char **argv)
         unlink(sockpath);
     }

-    if (device) {
+    if (device || find) {
         void *ret;
         pthread_join(client_thread, &ret);
         exit(ret != NULL);
Paolo Bonzini - Dec. 7, 2011, 9:44 a.m.
On 12/07/2011 05:23 AM, Chunyan Liu wrote:
> Add -f option to qemu-nbd. New implementation. Do not need nbd_setup.
> Tested and worked.
>
> Signed-off-by: Chunyan Liu <cyliu@suse.com <mailto:cyliu@suse.com>>
> ---
>   qemu-nbd.c |   71 +++++++++++++++++++++++++++++++++++++++++------------------
>   1 files changed, 49 insertions(+), 22 deletions(-)

Looks much nicer, thanks.

Paolo
Stefan Hajnoczi - Dec. 8, 2011, 11:55 a.m.
On Wed, Dec 7, 2011 at 4:23 AM, Chunyan Liu <cyliu@suse.com> 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 <anthony@codemonkey.ws>\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:

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

Patch

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 291cba2..ba66da1 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,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 <anthony@codemonkey.ws>\n"
-    , name, NBD_DEFAULT_PORT, "DEVICE");
+    , name, NBD_DEFAULT_PORT, "PID");
 }

 static void version(const char *name)
@@ -194,7 +194,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 +216,38 @@  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", device);
+            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)) {
+                continue;
+            }
+
+            fd = open(device, O_RDWR);
+            if (fd == -1 || nbd_init(fd, sock, nbdflags, size, blocksize)
== -1) {
+                free(device);
+                continue;
+            }
+
+            break;
+        }
+
+        if (i >= max_nbd) {
+            fprintf(stderr, "Fail to find a free nbd device\n");
+            goto out;
+        }
     }