| Message ID | 1322839676-7559-2-git-send-email-cyliu@suse.com |
|---|---|
| State | New |
| Headers | show |
On 12/02/2011 04:27 PM, Chunyan Liu wrote: > @@ -42,6 +42,18 @@ static int verbose; > static char *device; > static char *srcpath; > static char *sockpath; > +static int is_sockpath_option; > +static int sigterm_fd[2]; > +static off_t dev_offset; > +static uint32_t nbdflags; > +static bool disconnect; > +static const char *bindto = "0.0.0.0"; > +static int port = NBD_DEFAULT_PORT; > +static int li; > +static int flags = BDRV_O_RDWR; > +static int partition = -1; > +static int shared = 1; > +static int persistent; A lot of statics... "li" seems unused. I took patch 1/3 in my tree (git://github.com/bonzini/qemu.git branch nbd-server). I'll post it together with my patches next week. Paolo
2011/12/3 Paolo Bonzini <pbonzini@redhat.com> > On 12/02/2011 04:27 PM, Chunyan Liu wrote: > >> @@ -42,6 +42,18 @@ static int verbose; >> static char *device; >> static char *srcpath; >> static char *sockpath; >> +static int is_sockpath_option; >> +static int sigterm_fd[2]; >> +static off_t dev_offset; >> +static uint32_t nbdflags; >> +static bool disconnect; >> +static const char *bindto = "0.0.0.0"; >> +static int port = NBD_DEFAULT_PORT; >> +static int li; >> +static int flags = BDRV_O_RDWR; >> +static int partition = -1; >> +static int shared = 1; >> +static int persistent; >> > > A lot of statics... "li" seems unused. > Using these statics simply because most of them are global parameters getting from command line options, will be used later. Otherwise, the nbd_setup() function should take many parameters. Ahh, "li" could be defined in main(). After getting parameters from option, later places can use "port". case 'p': li = strtol(optarg, &end, 0); if (*end) { errx(EXIT_FAILURE, "Invalid port `%s'", optarg); } if (li < 1 || li > 65535) { errx(EXIT_FAILURE, "Port out of range `%s'", optarg); } port = (uint16_t)li; > I took patch 1/3 in my tree (git://github.com/bonzini/**qemu.git<http://github.com/bonzini/qemu.git>branch nbd-server). I'll post it together with my patches next week. > > Paolo > >
On Mon, Dec 5, 2011 at 5:46 AM, Chunyan Liu <cyliu@suse.com> wrote: > 2011/12/3 Paolo Bonzini <pbonzini@redhat.com> >> >> On 12/02/2011 04:27 PM, Chunyan Liu wrote: >>> >>> @@ -42,6 +42,18 @@ static int verbose; >>> static char *device; >>> static char *srcpath; >>> static char *sockpath; >>> +static int is_sockpath_option; >>> +static int sigterm_fd[2]; >>> +static off_t dev_offset; >>> +static uint32_t nbdflags; >>> +static bool disconnect; >>> +static const char *bindto = "0.0.0.0"; >>> +static int port = NBD_DEFAULT_PORT; >>> +static int li; >>> +static int flags = BDRV_O_RDWR; >>> +static int partition = -1; >>> +static int shared = 1; >>> +static int persistent; >> >> >> A lot of statics... "li" seems unused. > > > Using these statics simply because most of them are global parameters > getting from command line options, will be used later. Otherwise, the > nbd_setup() function should take many parameters. > > Ahh, "li" could be defined in main(). After getting parameters from option, > later places can use "port". > case 'p': > li = strtol(optarg, &end, 0); > if (*end) { > errx(EXIT_FAILURE, "Invalid port `%s'", optarg); > } > if (li < 1 || li > 65535) { > errx(EXIT_FAILURE, "Port out of range `%s'", optarg); > } > port = (uint16_t)li; You can also move the bdrv_new()/bdrv_open()/bdrv_delete() out of the nbd_setup() function. The sigterm_fd[] variable could also be handled more cleanly. Basically, I'd rather see a bigger patch that arranges things nicely than chopping the function in half and making most variables global in order to have a shorter patch. We need to maintain this code so if it needs to be rearranged in order to fit with the new requirements then that's worth doing so it will be easier to read and maintain in the future. nbd_setup() is a slightly confusing name because the entire main loop for the executes in the function. Perhaps nbd_run() is better. Stefan
2011/12/5 Stefan Hajnoczi <stefanha@gmail.com> > On Mon, Dec 5, 2011 at 5:46 AM, Chunyan Liu <cyliu@suse.com> wrote: > > 2011/12/3 Paolo Bonzini <pbonzini@redhat.com> > >> > >> On 12/02/2011 04:27 PM, Chunyan Liu wrote: > >>> > >>> @@ -42,6 +42,18 @@ static int verbose; > >>> static char *device; > >>> static char *srcpath; > >>> static char *sockpath; > >>> +static int is_sockpath_option; > >>> +static int sigterm_fd[2]; > >>> +static off_t dev_offset; > >>> +static uint32_t nbdflags; > >>> +static bool disconnect; > >>> +static const char *bindto = "0.0.0.0"; > >>> +static int port = NBD_DEFAULT_PORT; > >>> +static int li; > >>> +static int flags = BDRV_O_RDWR; > >>> +static int partition = -1; > >>> +static int shared = 1; > >>> +static int persistent; > >> > >> > >> A lot of statics... "li" seems unused. > > > > > > Using these statics simply because most of them are global parameters > > getting from command line options, will be used later. Otherwise, the > > nbd_setup() function should take many parameters. > > > > Ahh, "li" could be defined in main(). After getting parameters from > option, > > later places can use "port". > > case 'p': > > li = strtol(optarg, &end, 0); > > if (*end) { > > errx(EXIT_FAILURE, "Invalid port `%s'", optarg); > > } > > if (li < 1 || li > 65535) { > > errx(EXIT_FAILURE, "Port out of range `%s'", optarg); > > } > > port = (uint16_t)li; > > You can also move the bdrv_new()/bdrv_open()/bdrv_delete() out of the > nbd_setup() function. > Sure. But different parameters needed by nbd_setup(). (bs, dev_offset, fd_size instead of srcpath, dev_offset, partition, flags). The sigterm_fd[] variable could also be handled more cleanly. > Any suggestion? > Basically, I'd rather see a bigger patch that arranges things nicely > than chopping the function in half and making most variables global in > order to have a shorter patch. Currently, the nbd_setup needs parameters: device, srcpath, flags, partition, dev_offset, nbdflags, sockpath, bindto, port, shared, persistent, verbose, sigterm_rfd. More than 10 parameters. I still didn't find a better way to reduce parameters. Making variables global is a workaround to avoid nbd_setup taking too many parameters. Actually, except for sigterm_rfd, all others are pared from command line options. To improve, what I can think of is to define a structure to save those values parsed from command line options and pass that structure to nbd_setup as a parameter. Of course, bdrv_new/open/delete can be moved out, just passing more parameters to nbd_setup. Temporarily, I couldn't think of others. Do you have any better ideas? We need to maintain this code so if it > needs to be rearranged in order to fit with the new requirements then > that's worth doing so it will be easier to read and maintain in the > future. nbd_setup() is a slightly confusing name because the entire main loop > for the executes in the function. Perhaps nbd_run() is better. No problem, could be changed. > Stefan > >
On 12/06/2011 07:56 AM, Chunyan Liu wrote: > > Currently, the nbd_setup needs parameters: device, srcpath, flags, > partition, dev_offset, nbdflags, sockpath, bindto, port, shared, > persistent, verbose, sigterm_rfd. More than 10 parameters. I still > didn't find a better way to reduce parameters. Making variables global > is a workaround to avoid nbd_setup taking too many parameters. Actually, > except for sigterm_rfd, all others are pared from command line options. Reading again this patch, I am not sure why you are doing it this way. There is no reason why bdrv_new/open/delete has to be redone for every /dev/nbdX we try (or if there is a reason, _that_ is what should be fixed first). Also the "tail" of nbd_setup, basically the select loop, should not be tried multiple times. I do not understand why you cannot simply do it like this: - in the server thread, do everything as it is now - pass "device" to the client thread instead of opening it in main() - in the client thread, either use "device" as it is or (if device == NULL, which implies find == 1) loop until nbd_init succeeds. Am I just confused? Paolo
2011/12/6 Paolo Bonzini <pbonzini@redhat.com> > On 12/06/2011 07:56 AM, Chunyan Liu wrote: > >> >> Currently, the nbd_setup needs parameters: device, srcpath, flags, >> partition, dev_offset, nbdflags, sockpath, bindto, port, shared, >> persistent, verbose, sigterm_rfd. More than 10 parameters. I still >> didn't find a better way to reduce parameters. Making variables global >> is a workaround to avoid nbd_setup taking too many parameters. Actually, >> except for sigterm_rfd, all others are pared from command line options. >> > > Reading again this patch, I am not sure why you are doing it this way. > > There is no reason why bdrv_new/open/delete has to be redone for every > /dev/nbdX we try (or if there is a reason, _that_ is what should be fixed > first). Well, it's not "had to be redone", did it just to avoid passing too many parameters to nbd_setup. (otherwise, should pass "bs, dev_offset and fd_size to nbd_setup.) Can be moved out. > Also the "tail" of nbd_setup, basically the select loop, should not be > tried multiple times. > > I do not understand why you cannot simply do it like this: > > - in the server thread, do everything as it is now > Nope. When device changes, both client thread and server thread should be refreshed. sockpath and sharing_fds[] is changed with different device. > - pass "device" to the client thread instead of opening it in main() > > - in the client thread, either use "device" as it is or (if device == > NULL, which implies find == 1) loop until nbd_init succeeds. Am I just confused? > > Paolo > >
On 12/06/2011 09:42 AM, Chunyan Liu wrote: > > I do not understand why you cannot simply do it like this: > > - in the server thread, do everything as it is now > > Nope. When device changes, both client thread and server thread should > be refreshed. sockpath and sharing_fds[] is changed with different device. Then let's change the default sockpath to include the pid rather than the NBD device name. Paolo
2011/12/6 Paolo Bonzini <pbonzini@redhat.com> > On 12/06/2011 09:42 AM, Chunyan Liu wrote: > >> >> I do not understand why you cannot simply do it like this: >> >> - in the server thread, do everything as it is now >> >> Nope. When device changes, both client thread and server thread should >> be refreshed. sockpath and sharing_fds[] is changed with different device. >> > > Then let's change the default sockpath to include the pid rather than the > NBD device name. > > Sounds good. Will try. Thanks. > Paolo > >
diff --git a/qemu-nbd.c b/qemu-nbd.c index 291cba2..83ae30f 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -42,6 +42,18 @@ static int verbose; static char *device; static char *srcpath; static char *sockpath; +static int is_sockpath_option; +static int sigterm_fd[2]; +static off_t dev_offset; +static uint32_t nbdflags; +static bool disconnect; +static const char *bindto = "0.0.0.0"; +static int port = NBD_DEFAULT_PORT; +static int li; +static int flags = BDRV_O_RDWR; +static int partition = -1; +static int shared = 1; +static int persistent; static void usage(const char *name) { @@ -171,9 +183,7 @@ static int find_partition(BlockDriverState *bs, int partition, static void termsig_handler(int signum) { static int sigterm_reported; - if (!sigterm_reported) { - sigterm_reported = (write(sigterm_wfd, "", 1) == 1); - } + sigterm_reported = (write(sigterm_wfd, "", 1) == 1); } static void *show_parts(void *arg) @@ -244,18 +254,152 @@ out: return (void *) EXIT_FAILURE; } -int main(int argc, char **argv) +static int nbd_setup(void) { BlockDriverState *bs; - off_t dev_offset = 0; off_t offset = 0; - uint32_t nbdflags = 0; - bool disconnect = false; - const char *bindto = "0.0.0.0"; - int port = NBD_DEFAULT_PORT; struct sockaddr_in addr; socklen_t addr_len = sizeof(addr); off_t fd_size; + uint8_t *data; + fd_set fds; + int *sharing_fds; + int fd; + int i; + int nb_fds = 0; + int max_fd; + pthread_t client_thread; + int ret; + + 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 (sockpath == NULL) { + sockpath = g_malloc(128); + snprintf(sockpath, 128, SOCKET_PATH, basename(device)); + } + } + + bs = bdrv_new("hda"); + if ((ret = bdrv_open(bs, srcpath, flags, NULL)) < 0) { + errno = -ret; + err(EXIT_FAILURE, "Failed to bdrv_open '%s'", srcpath); + } + + 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) { + sharing_fds[0] = unix_socket_incoming(sockpath); + } else { + sharing_fds[0] = tcp_socket_incoming(bindto, port); + } + + if (sharing_fds[0] == -1) + return 1; + + if (device) { + int ret; + + ret = pthread_create(&client_thread, NULL, nbd_client_thread, &fd); + if (ret != 0) { + errx(EXIT_FAILURE, "Failed to create client thread: %s", + strerror(ret)); + } + } else { + /* Shut up GCC warnings. */ + memset(&client_thread, 0, sizeof(client_thread)); + } + + max_fd = sharing_fds[0]; + nb_fds++; + + data = qemu_blockalign(bs, NBD_BUFFER_SIZE); + if (data == NULL) { + errx(EXIT_FAILURE, "Cannot allocate data buffer"); + } + + do { + FD_ZERO(&fds); + FD_SET(sigterm_fd[0], &fds); + for (i = 0; i < nb_fds; i++) + FD_SET(sharing_fds[i], &fds); + + do { + 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)) + 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, + &offset, nbdflags, data, NBD_BUFFER_SIZE) != 0) { + close(sharing_fds[i]); + nb_fds--; + sharing_fds[i] = sharing_fds[nb_fds]; + i--; + } + ret--; + } + } + /* new connection ? */ + if (FD_ISSET(sharing_fds[0], &fds)) { + if (nb_fds < shared + 1) { + sharing_fds[nb_fds] = accept(sharing_fds[0], + (struct sockaddr *)&addr, + &addr_len); + if (sharing_fds[nb_fds] != -1 && + nbd_negotiate(sharing_fds[nb_fds], fd_size, nbdflags) != -1) { + if (sharing_fds[nb_fds] > max_fd) + max_fd = sharing_fds[nb_fds]; + nb_fds++; + } + } + } + } while (persistent || nb_fds > 1); + qemu_vfree(data); + + close(sharing_fds[0]); + g_free(sharing_fds); + if (sockpath) { + unlink(sockpath); + } + if (bs) { + bdrv_delete(bs); + } + if (!is_sockpath_option) { + sockpath = NULL; + } + + if (device) { + void *ret; + pthread_join(client_thread, &ret); + return (ret != NULL) ? EXIT_FAILURE : EXIT_SUCCESS; + } else { + return EXIT_SUCCESS; + } +} + +int main(int argc, char **argv) +{ const char *sopt = "hVb:o:p:rsnP:c:dvk:e:t"; struct option lopt[] = { { "help", 0, NULL, 'h' }, @@ -277,27 +421,14 @@ int main(int argc, char **argv) }; int ch; int opt_ind = 0; - int li; char *end; - int flags = BDRV_O_RDWR; - int partition = -1; - int ret; - int shared = 1; - uint8_t *data; - fd_set fds; - int *sharing_fds; - int fd; - int i; - int nb_fds = 0; - int max_fd; - int persistent = 0; - pthread_t client_thread; + int ret = 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. */ struct sigaction sa_sigterm; - int sigterm_fd[2]; + if (qemu_pipe(sigterm_fd) == -1) { err(EXIT_FAILURE, "Error setting up communication pipe"); } @@ -352,6 +483,7 @@ int main(int argc, char **argv) sockpath = optarg; if (sockpath[0] != '/') errx(EXIT_FAILURE, "socket path must be absolute\n"); + is_sockpath_option = 1; break; case 'd': disconnect = true; @@ -395,6 +527,7 @@ int main(int argc, char **argv) } if (disconnect) { + int fd; fd = open(argv[optind], O_RDWR); if (fd == -1) err(EXIT_FAILURE, "Cannot open %s", argv[optind]); @@ -411,7 +544,6 @@ int main(int argc, char **argv) if (device && !verbose) { int stderr_fd[2]; pid_t pid; - int ret; if (qemu_pipe(stderr_fd) == -1) { err(EXIT_FAILURE, "Error setting up communication pipe"); @@ -460,125 +592,11 @@ 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 (sockpath == NULL) { - sockpath = g_malloc(128); - snprintf(sockpath, 128, SOCKET_PATH, basename(device)); - } - } - 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) { - sharing_fds[0] = unix_socket_incoming(sockpath); - } else { - sharing_fds[0] = tcp_socket_incoming(bindto, port); - } - - if (sharing_fds[0] == -1) - return 1; - - if (device) { - int ret; - - ret = pthread_create(&client_thread, NULL, nbd_client_thread, &fd); - if (ret != 0) { - errx(EXIT_FAILURE, "Failed to create client thread: %s", - strerror(ret)); - } - } else { - /* Shut up GCC warnings. */ - memset(&client_thread, 0, sizeof(client_thread)); - } - - max_fd = sharing_fds[0]; - nb_fds++; - - data = qemu_blockalign(bs, NBD_BUFFER_SIZE); - if (data == NULL) { - errx(EXIT_FAILURE, "Cannot allocate data buffer"); - } - - do { - FD_ZERO(&fds); - FD_SET(sigterm_fd[0], &fds); - for (i = 0; i < nb_fds; i++) - FD_SET(sharing_fds[i], &fds); - - do { - ret = select(max_fd + 1, &fds, NULL, NULL, NULL); - } while (ret == -1 && errno == EINTR); - if (ret == -1 || FD_ISSET(sigterm_fd[0], &fds)) { - break; - } - - 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, - &offset, nbdflags, data, NBD_BUFFER_SIZE) != 0) { - close(sharing_fds[i]); - nb_fds--; - sharing_fds[i] = sharing_fds[nb_fds]; - i--; - } - ret--; - } - } - /* new connection ? */ - if (FD_ISSET(sharing_fds[0], &fds)) { - if (nb_fds < shared + 1) { - sharing_fds[nb_fds] = accept(sharing_fds[0], - (struct sockaddr *)&addr, - &addr_len); - if (sharing_fds[nb_fds] != -1 && - nbd_negotiate(sharing_fds[nb_fds], fd_size, nbdflags) != -1) { - if (sharing_fds[nb_fds] > max_fd) - max_fd = sharing_fds[nb_fds]; - nb_fds++; - } - } - } - } while (persistent || nb_fds > 1); - qemu_vfree(data); - close(sharing_fds[0]); - g_free(sharing_fds); - if (sockpath) { - unlink(sockpath); - } + ret = nbd_setup(); - if (device) { - void *ret; - pthread_join(client_thread, &ret); - exit(ret != NULL); - } else { - exit(EXIT_SUCCESS); - } + exit(ret); }
According to Stefan's suggestion, will loop over /dev/nbd%d to do "qemu-nbd -f disk.img", if fails, try next device. To make "qemu-nbd -c" and "qemu-nbd -f" share codes as more as possible, extract the shared codes to a function nbd_setup(). Current qemu-nbd functions work well still. Signed-off-by: Chunyan Liu <cyliu@suse.com> --- qemu-nbd.c | 300 ++++++++++++++++++++++++++++++++---------------------------- 1 files changed, 159 insertions(+), 141 deletions(-)