[Qemu-devel] [PATCH V3 1/2] Update ioctl order in nbd_init to detect -EBUSY

[Qemu-devel] [PATCH V3 1/2] Update ioctl order in nbd_init to detect -EBUSY

[Chunyan Liu]

Remove NBD_CLEAR_SOCK from nbd_init and adjust NBD_SET_SOCK ioctl before other
ioctl(s) in nbd_init so that it can detect -EBUSY properly.
 
Signed-off-by: Chunyan Liu <cyliu@suse.com>
---
 nbd.c |   27 +++++++++------------------
 1 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/nbd.c b/nbd.c
index e6c931c..4184614 100644
--- a/nbd.c
+++ b/nbd.c
@@ -348,6 +348,15 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
 #ifdef __linux__
 int nbd_init(int fd, int csock, uint32_t flags, off_t size, size_t blocksize)
 {
+    TRACE("Setting NBD socket");
+
+    if (ioctl(fd, NBD_SET_SOCK, csock) == -1) {
+        int serrno = errno;
+        LOG("Failed to set NBD socket");
+        errno = serrno;
+        return -1;
+    }
+
     TRACE("Setting block size to %lu", (unsigned long)blocksize);
 
     if (ioctl(fd, NBD_SET_BLKSIZE, blocksize) == -1) {
@@ -386,24 +395,6 @@ int nbd_init(int fd, int csock, uint32_t flags, off_t size, size_t blocksize)
         return -1;
     }
 
-    TRACE("Clearing NBD socket");
-
-    if (ioctl(fd, NBD_CLEAR_SOCK) == -1) {
-        int serrno = errno;
-        LOG("Failed clearing NBD socket");
-        errno = serrno;
-        return -1;
-    }
-
-    TRACE("Setting NBD socket");
-
-    if (ioctl(fd, NBD_SET_SOCK, csock) == -1) {
-        int serrno = errno;
-        LOG("Failed to set NBD socket");
-        errno = serrno;
-        return -1;
-    }
-
     TRACE("Negotiation ended");
 
     return 0;
-- 
1.7.3.4

[Qemu-devel] [PATCH V3 2/2] Add -f option to qemu-nbd

[Chunyan Liu]

V3:
Remove file lock in main(). 
Try to find new free nbd device and connect to it if connecting to the first
first found free nbd device failed.

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

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 291cba2..fcaaf50 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -32,6 +32,7 @@
 #include <signal.h>
 #include <libgen.h>
 #include <pthread.h>
+#include <fcntl.h>
 
 #define SOCKET_PATH    "/var/lock/qemu-nbd-%s"
 
@@ -244,6 +245,60 @@ out:
     return (void *) EXIT_FAILURE;
 }
 
+static int is_nbd_used(int minor)
+{
+    FILE *proc;
+    int NBDMAJOR = 43;
+    char buf[BUFSIZ];
+    int find = 0;
+
+    proc = fopen("/proc/partitions", "r");
+    if (proc != NULL) {
+        while (fgets(buf, sizeof(buf), proc)) {
+            int m, n;
+            unsigned long long sz;
+            char name[16];
+            char *pname = name;
+            char *end;
+
+            if (sscanf(buf, " %d %d %llu %128[^\n ]",
+                    &m, &n, &sz, name) != 4)
+                continue;
+            if (m != NBDMAJOR)
+                continue;
+            if (strncmp(name, "nbd", 3))
+                continue;
+            pname += 3;
+            n = strtol(pname, &end, 10);
+            if (end && end != pname && *end == '\0' && n == minor) {
+                find = 1;
+                break;
+            }
+        }
+        fclose(proc);
+    }
+
+    return find;
+}
+
+static char *find_free_nbd_device(void)
+{
+    int i;
+    int nbds_max = 16;
+    char name[16];
+    char *devname = NULL;
+
+    for (i = 0; i < nbds_max; i++) {
+        if (!is_nbd_used(i)) {
+            snprintf(name, sizeof(name), "/dev/nbd%d", i);
+            devname = strdup(name);
+            break;
+        }
+    }
+
+    return devname;
+}
+
 int main(int argc, char **argv)
 {
     BlockDriverState *bs;
@@ -256,7 +311,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 +328,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;
@@ -292,6 +348,8 @@ int main(int argc, char **argv)
     int max_fd;
     int persistent = 0;
     pthread_t client_thread;
+    char *devname = NULL;
+    int find = 0;
 
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -374,6 +432,18 @@ int main(int argc, char **argv)
         case 'v':
             verbose = 1;
             break;
+        case 'f':
+            find = 1;
+            devname = find_free_nbd_device();
+            if (devname == NULL)
+                exit(1);
+            if (argc == optind) {
+                printf("%s\n", devname);
+                free(devname);
+                exit(0);
+            }
+            device = devname;
+            break;
         case 'V':
             version(argv[0]);
             exit(0);
@@ -464,6 +534,7 @@ int main(int argc, char **argv)
         /* Open before spawning new threads.  In the future, we may
          * drop privileges after opening.
          */
+retry:
         fd = open(device, O_RDWR);
         if (fd == -1) {
             err(EXIT_FAILURE, "Failed to open %s", device);
@@ -577,6 +648,13 @@ int main(int argc, char **argv)
     if (device) {
         void *ret;
         pthread_join(client_thread, &ret);
+        if (ret != EXIT_SUCCESS && find) {
+            free(device);
+            device = find_free_nbd_device();
+            if (!device)
+                err(EXIT_FAILURE, "Could not find free nbd device");
+            goto retry;
+        }
         exit(ret != NULL);
     } else {
         exit(EXIT_SUCCESS);
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH V3 2/2] Add -f option to qemu-nbd

[Stefan Hajnoczi]

On Wed, Nov 23, 2011 at 10:14 AM, Chunyan Liu <cyliu@suse.com> wrote:
> V3:
> Remove file lock in main().
> Try to find new free nbd device and connect to it if connecting to the first
> first found free nbd device failed.
>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  qemu-nbd.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 79 insertions(+), 1 deletions(-)

I not seeing the part where you adjusted the ioctl order.

The /proc/partitions scanning is unnecessary since we can just loop
over /dev/ndb%d and try to initialize.  If a device is in use then
init will fail and we need to try the next one.  If a device is free
we can continue with normal operation.  I guess I'm saying that once
you fix the ioctl order then there's no need for another mechanism to
test whether or not a device is in use.

Also please use QEMU coding style, you can run
qemu/scripts/checkpatch.pl on your patches.

Stefan

Re: [Qemu-devel] [PATCH V3 2/2] Add -f option to qemu-nbd

[Stefan Hajnoczi]

On Wed, Nov 23, 2011 at 10:56 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Wed, Nov 23, 2011 at 10:14 AM, Chunyan Liu <cyliu@suse.com> wrote:
>> V3:
>> Remove file lock in main().
>> Try to find new free nbd device and connect to it if connecting to the first
>> first found free nbd device failed.
>>
>> Signed-off-by: Chunyan Liu <cyliu@suse.com>
>> ---
>>  qemu-nbd.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 79 insertions(+), 1 deletions(-)
>
> I not seeing the part where you adjusted the ioctl order.

I see the other patch on the qemu-devel list now, sorry.  Looked at
this mail first because it was CCed to me.

Stefan

Re: [Qemu-devel] [PATCH V3 2/2] Add -f option to qemu-nbd

[Chunyan Liu]

[-- Attachment #1: Type: text/plain, Size: 1443 bytes --]

2011/11/23 Stefan Hajnoczi <stefanha@gmail.com>

> On Wed, Nov 23, 2011 at 10:14 AM, Chunyan Liu <cyliu@suse.com> wrote:
> > V3:
> > Remove file lock in main().
> > Try to find new free nbd device and connect to it if connecting to the
> first
> > first found free nbd device failed.
> >
> > Signed-off-by: Chunyan Liu <cyliu@suse.com>
> > ---
> >  qemu-nbd.c |   80
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 79 insertions(+), 1 deletions(-)
>
> I not seeing the part where you adjusted the ioctl order.
>
> The /proc/partitions scanning is unnecessary since we can just loop
> over /dev/ndb%d and try to initialize.  If a device is in use then
> init will fail and we need to try the next one.  If a device is free
> we can continue with normal operation.  I guess I'm saying that once
> you fix the ioctl order then there's no need for another mechanism to
> test whether or not a device is in use.
>

The way of scanning /proc/partitions to find an unused nbd device first can
borrow the code for "qemu-nbd -c" to do the left things. .
The way of loop over /dev/nbd%d and try to initialize, from the first
thought, needs do all things in the loop, including handling -v, nbd_init,
nbd_client, etc. That part of code is quite similar to "qemu-nbd -c". I
don't know if that is better?



>

Also please use QEMU coding style, you can run
> qemu/scripts/checkpatch.pl on your patches.
>
> Stefan
>
>

[-- Attachment #2: Type: text/html, Size: 2329 bytes --]

Re: [Qemu-devel] [PATCH V3 2/2] Add -f option to qemu-nbd

[Stefan Hajnoczi]

On Thu, Nov 24, 2011 at 3:38 AM, Chunyan Liu <cyliu@suse.com> wrote:
>
>
> 2011/11/23 Stefan Hajnoczi <stefanha@gmail.com>
>>
>> On Wed, Nov 23, 2011 at 10:14 AM, Chunyan Liu <cyliu@suse.com> wrote:
>> > V3:
>> > Remove file lock in main().
>> > Try to find new free nbd device and connect to it if connecting to the
>> > first
>> > first found free nbd device failed.
>> >
>> > Signed-off-by: Chunyan Liu <cyliu@suse.com>
>> > ---
>> >  qemu-nbd.c |   80
>> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> >  1 files changed, 79 insertions(+), 1 deletions(-)
>>
>> I not seeing the part where you adjusted the ioctl order.
>>
>> The /proc/partitions scanning is unnecessary since we can just loop
>> over /dev/ndb%d and try to initialize.  If a device is in use then
>> init will fail and we need to try the next one.  If a device is free
>> we can continue with normal operation.  I guess I'm saying that once
>> you fix the ioctl order then there's no need for another mechanism to
>> test whether or not a device is in use.
>
> The way of scanning /proc/partitions to find an unused nbd device first can
> borrow the code for "qemu-nbd -c" to do the left things. .
> The way of loop over /dev/nbd%d and try to initialize, from the first
> thought, needs do all things in the loop, including handling -v, nbd_init,
> nbd_client, etc. That part of code is quite similar to "qemu-nbd -c". I
> don't know if that is better?

This might be a chance to refactor the code slightly, that would also
avoid you having to introduce a goto retry.

Stefan

Re: [Qemu-devel] [PATCH V3 2/2] Add -f option to qemu-nbd

[Chunyan Liu]

[-- Attachment #1.1: Type: text/plain, Size: 2987 bytes --]

2011/11/24 Stefan Hajnoczi <stefanha@gmail.com>

> On Thu, Nov 24, 2011 at 3:38 AM, Chunyan Liu <cyliu@suse.com> wrote:
> >
> >
> > 2011/11/23 Stefan Hajnoczi <stefanha@gmail.com>
> >>
> >> On Wed, Nov 23, 2011 at 10:14 AM, Chunyan Liu <cyliu@suse.com> wrote:
> >> > V3:
> >> > Remove file lock in main().
> >> > Try to find new free nbd device and connect to it if connecting to the
> >> > first
> >> > first found free nbd device failed.
> >> >
> >> > Signed-off-by: Chunyan Liu <cyliu@suse.com>
> >> > ---
> >> >  qemu-nbd.c |   80
> >> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >> >  1 files changed, 79 insertions(+), 1 deletions(-)
> >>
> >> I not seeing the part where you adjusted the ioctl order.
> >>
> >> The /proc/partitions scanning is unnecessary since we can just loop
> >> over /dev/ndb%d and try to initialize.  If a device is in use then
> >> init will fail and we need to try the next one.  If a device is free
> >> we can continue with normal operation.  I guess I'm saying that once
> >> you fix the ioctl order then there's no need for another mechanism to
> >> test whether or not a device is in use.
> >
> > The way of scanning /proc/partitions to find an unused nbd device first
> can
> > borrow the code for "qemu-nbd -c" to do the left things. .
> > The way of loop over /dev/nbd%d and try to initialize, from the first
> > thought, needs do all things in the loop, including handling -v,
> nbd_init,
> > nbd_client, etc. That part of code is quite similar to "qemu-nbd -c". I
> > don't know if that is better?
>
> This might be a chance to refactor the code slightly, that would also
> avoid you having to introduce a goto retry.
>
> About detail implementation of the loop over /dev/nbd%d way, have done
some coding and testing. I'm afraid the reorganization work is not very
slight, not sure how do you think.
First, through nbd_init() success or fail to check if the device is in use,
at least nbd_init() and operations before nbd_init() but device relative
should be in the loop. Usually, there are two ways to be considered:
1. Try to divide current "qemu-nbd -c" processing code into two parts (
nbd_init() and before, and operations after nbd_init()), 1st part put into
the loop, 2nd part outside loop. This way is hard to achieve. nbd_init()
should be in client thread, but the loop is outside that thread, it's not
proper to drag out nbd_init() from client thread and put it into the loop.
2. Put all "qemu-nbd -c" processing code (device relative) in the loop,
directly check the "qemu-nbd -c /dev/nbd%d disk.img" result, if fail, try
next nbd device. In this way, will adjust current code order and extract
device related codes to a new function so that both "qemu-nbd -c" and
"qemu-nbd -f" can use.(some initialization work and cleanup work to be
cared)

A draft is in attachment. qemu-nbd -f is working, since I'm not sure such
change is acceptable or not, not extract code in the loop into a function
yet.


>  Stefan
>
>

[-- Attachment #1.2: Type: text/html, Size: 4177 bytes --]

[-- Attachment #2: test_find.patch --]
[-- Type: text/x-patch, Size: 4369 bytes --]

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 291cba2..38c3bc3 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -171,9 +171,9 @@ static int find_partition(BlockDriverState *bs, int partition,
 static void termsig_handler(int signum)
 {
     static int sigterm_reported;
-    if (!sigterm_reported) {
+//    if (!sigterm_reported) {
         sigterm_reported = (write(sigterm_wfd, "", 1) == 1);
-    }
+//    }
 }
 
 static void *show_parts(void *arg)
@@ -256,7 +256,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 +273,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;
@@ -292,7 +293,8 @@ int main(int argc, char **argv)
     int max_fd;
     int persistent = 0;
     pthread_t client_thread;
-
+    int find = 0;
+    int find_minor;
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
      */
@@ -374,6 +376,10 @@ int main(int argc, char **argv)
         case 'v':
             verbose = 1;
             break;
+        case 'f':
+            find = 1;
+            device = "/dev/nbd0";
+            break;
         case 'V':
             version(argv[0]);
             exit(0);
@@ -459,11 +465,34 @@ int main(int argc, char **argv)
             exit(errors);
         }
     }
+    bdrv_init();
+    atexit(bdrv_close_all);
+
+    bs = bdrv_new("hdb");
+    srcpath = argv[optind];
+    if ((ret = bdrv_open(bs, srcpath, flags, NULL)) < 0) {
+        errno = -ret;
+        err(EXIT_FAILURE, "Failed to bdrv_open '%s'", argv[optind]);
+    }
 
+    fd_size = bs->total_sectors * 512;
+    if (partition != -1 &&
+        find_partition(bs, partition, &dev_offset, &fd_size)) {
+        err(EXIT_FAILURE, "Could not find partition %d", partition);
+    }
+ 
+if (find) {
+    for (find_minor=0; find_minor<16; find_minor++){
+    asprintf(&device, "/dev/nbd%d", find_minor);
+    if (!device) {
+        continue;
+    }
+ 
     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);
@@ -475,23 +504,6 @@ int main(int argc, char **argv)
         }
     }
 
-    bdrv_init();
-    atexit(bdrv_close_all);
-
-    bs = bdrv_new("hda");
-    srcpath = argv[optind];
-    if ((ret = bdrv_open(bs, srcpath, flags, NULL)) < 0) {
-        errno = -ret;
-        err(EXIT_FAILURE, "Failed to bdrv_open '%s'", argv[optind]);
-    }
-
-    fd_size = bs->total_sectors * 512;
-
-    if (partition != -1 &&
-        find_partition(bs, partition, &dev_offset, &fd_size)) {
-        err(EXIT_FAILURE, "Could not find partition %d", partition);
-    }
-
     sharing_fds = g_malloc((shared + 1) * sizeof(int));
 
     if (sockpath) {
@@ -534,11 +546,14 @@ int main(int argc, char **argv)
             ret = select(max_fd + 1, &fds, NULL, NULL, NULL);
         } while (ret == -1 && errno == EINTR);
         if (ret == -1 || FD_ISSET(sigterm_fd[0], &fds)) {
+            int sigbuf[1];
+            read(sigterm_fd[0], sigbuf, 1);
             break;
         }
 
-        if (FD_ISSET(sharing_fds[0], &fds))
+        if (FD_ISSET(sharing_fds[0], &fds)) 
             ret--;
+        
         for (i = 1; i < nb_fds && ret; i++) {
             if (FD_ISSET(sharing_fds[i], &fds)) {
                 if (nbd_trip(bs, sharing_fds[i], fd_size, dev_offset,
@@ -577,8 +592,18 @@ int main(int argc, char **argv)
     if (device) {
         void *ret;
         pthread_join(client_thread, &ret);
-        exit(ret != NULL);
+        if (ret != EXIT_SUCCESS && find) {
+            sockpath = NULL;
+            nb_fds = 0;
+            free(device);
+            continue;
+        }
+        else {
+            exit(ret != NULL);
+        }
     } else {
         exit(EXIT_SUCCESS);
     }
+    }
+}
 }

Re: [Qemu-devel] [PATCH V3 2/2] Add -f option to qemu-nbd

[Stefan Hajnoczi]

On Fri, Nov 25, 2011 at 10:19 AM, Chunyan Liu <cyliu@suse.com> wrote:
>
>
> 2011/11/24 Stefan Hajnoczi <stefanha@gmail.com>
>>
>> On Thu, Nov 24, 2011 at 3:38 AM, Chunyan Liu <cyliu@suse.com> wrote:
>> >
>> >
>> > 2011/11/23 Stefan Hajnoczi <stefanha@gmail.com>
>> >>
>> >> On Wed, Nov 23, 2011 at 10:14 AM, Chunyan Liu <cyliu@suse.com> wrote:
>> >> > V3:
>> >> > Remove file lock in main().
>> >> > Try to find new free nbd device and connect to it if connecting to
>> >> > the
>> >> > first
>> >> > first found free nbd device failed.
>> >> >
>> >> > Signed-off-by: Chunyan Liu <cyliu@suse.com>
>> >> > ---
>> >> >  qemu-nbd.c |   80
>> >> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> >> >  1 files changed, 79 insertions(+), 1 deletions(-)
>> >>
>> >> I not seeing the part where you adjusted the ioctl order.
>> >>
>> >> The /proc/partitions scanning is unnecessary since we can just loop
>> >> over /dev/ndb%d and try to initialize.  If a device is in use then
>> >> init will fail and we need to try the next one.  If a device is free
>> >> we can continue with normal operation.  I guess I'm saying that once
>> >> you fix the ioctl order then there's no need for another mechanism to
>> >> test whether or not a device is in use.
>> >
>> > The way of scanning /proc/partitions to find an unused nbd device first
>> > can
>> > borrow the code for "qemu-nbd -c" to do the left things. .
>> > The way of loop over /dev/nbd%d and try to initialize, from the first
>> > thought, needs do all things in the loop, including handling -v,
>> > nbd_init,
>> > nbd_client, etc. That part of code is quite similar to "qemu-nbd -c". I
>> > don't know if that is better?
>>
>> This might be a chance to refactor the code slightly, that would also
>> avoid you having to introduce a goto retry.
>>
> About detail implementation of the loop over /dev/nbd%d way, have done some
> coding and testing. I'm afraid the reorganization work is not very slight,
> not sure how do you think.
> First, through nbd_init() success or fail to check if the device is in use,
> at least nbd_init() and operations before nbd_init() but device relative
> should be in the loop. Usually, there are two ways to be considered:
> 1. Try to divide current "qemu-nbd -c" processing code into two parts (
> nbd_init() and before, and operations after nbd_init()), 1st part put into
> the loop, 2nd part outside loop. This way is hard to achieve. nbd_init()
> should be in client thread, but the loop is outside that thread, it's not
> proper to drag out nbd_init() from client thread and put it into the loop.
> 2. Put all "qemu-nbd -c" processing code (device relative) in the loop,
> directly check the "qemu-nbd -c /dev/nbd%d disk.img" result, if fail, try
> next nbd device. In this way, will adjust current code order and extract
> device related codes to a new function so that both "qemu-nbd -c" and
> "qemu-nbd -f" can use.(some initialization work and cleanup work to be
> cared)
>
> A draft is in attachment. qemu-nbd -f is working, since I'm not sure such
> change is acceptable or not, not extract code in the loop into a function
> yet.

I think a common function makes sense.  This may be nicest in two
patches, Patch 1 moves the code into a function, Patch 2 implements
-f/find.

Stefan

Re: [Qemu-devel] [PATCH V3 2/2] Add -f option to qemu-nbd

[Chunyan Liu]

[-- Attachment #1: Type: text/plain, Size: 3722 bytes --]

2011/11/29 Stefan Hajnoczi <stefanha@gmail.com>

> On Fri, Nov 25, 2011 at 10:19 AM, Chunyan Liu <cyliu@suse.com> wrote:
> >
> >
> > 2011/11/24 Stefan Hajnoczi <stefanha@gmail.com>
> >>
> >> On Thu, Nov 24, 2011 at 3:38 AM, Chunyan Liu <cyliu@suse.com> wrote:
> >> >
> >> >
> >> > 2011/11/23 Stefan Hajnoczi <stefanha@gmail.com>
> >> >>
> >> >> On Wed, Nov 23, 2011 at 10:14 AM, Chunyan Liu <cyliu@suse.com>
> wrote:
> >> >> > V3:
> >> >> > Remove file lock in main().
> >> >> > Try to find new free nbd device and connect to it if connecting to
> >> >> > the
> >> >> > first
> >> >> > first found free nbd device failed.
> >> >> >
> >> >> > Signed-off-by: Chunyan Liu <cyliu@suse.com>
> >> >> > ---
> >> >> >  qemu-nbd.c |   80
> >> >> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >> >> >  1 files changed, 79 insertions(+), 1 deletions(-)
> >> >>
> >> >> I not seeing the part where you adjusted the ioctl order.
> >> >>
> >> >> The /proc/partitions scanning is unnecessary since we can just loop
> >> >> over /dev/ndb%d and try to initialize.  If a device is in use then
> >> >> init will fail and we need to try the next one.  If a device is free
> >> >> we can continue with normal operation.  I guess I'm saying that once
> >> >> you fix the ioctl order then there's no need for another mechanism to
> >> >> test whether or not a device is in use.
> >> >
> >> > The way of scanning /proc/partitions to find an unused nbd device
> first
> >> > can
> >> > borrow the code for "qemu-nbd -c" to do the left things. .
> >> > The way of loop over /dev/nbd%d and try to initialize, from the first
> >> > thought, needs do all things in the loop, including handling -v,
> >> > nbd_init,
> >> > nbd_client, etc. That part of code is quite similar to "qemu-nbd -c".
> I
> >> > don't know if that is better?
> >>
> >> This might be a chance to refactor the code slightly, that would also
> >> avoid you having to introduce a goto retry.
> >>
> > About detail implementation of the loop over /dev/nbd%d way, have done
> some
> > coding and testing. I'm afraid the reorganization work is not very
> slight,
> > not sure how do you think.
> > First, through nbd_init() success or fail to check if the device is in
> use,
> > at least nbd_init() and operations before nbd_init() but device relative
> > should be in the loop. Usually, there are two ways to be considered:
> > 1. Try to divide current "qemu-nbd -c" processing code into two parts (
> > nbd_init() and before, and operations after nbd_init()), 1st part put
> into
> > the loop, 2nd part outside loop. This way is hard to achieve. nbd_init()
> > should be in client thread, but the loop is outside that thread, it's not
> > proper to drag out nbd_init() from client thread and put it into the
> loop.
> > 2. Put all "qemu-nbd -c" processing code (device relative) in the loop,
> > directly check the "qemu-nbd -c /dev/nbd%d disk.img" result, if fail, try
> > next nbd device. In this way, will adjust current code order and extract
> > device related codes to a new function so that both "qemu-nbd -c" and
> > "qemu-nbd -f" can use.(some initialization work and cleanup work to be
> > cared)
> >
> > A draft is in attachment. qemu-nbd -f is working, since I'm not sure such
> > change is acceptable or not, not extract code in the loop into a function
> > yet.
>
> I think a common function makes sense.  This may be nicest in two
> patches, Patch 1 moves the code into a function, Patch 2 implements
> -f/find.
>
>
Just sent the patch sets according to above suggestion. Please review.
1/3-Update-ioctl-order-in-nbd_init-to-detect-EBUSY
2/3-Extract-code-to-nbd_setup-function-to-be-used-for-mamy-purposes
3/3-Add-f-option-to-qemu-nbd

Stefan
>
>

[-- Attachment #2: Type: text/html, Size: 5388 bytes --]