| Message ID | CAERYnoa9cYaRXvvju6B9m-L1h3oXL1yxdD19zer1VLmcAh4TiA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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 > >
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); } + } +} }