From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:42707) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RnT7E-0000LB-Ik for qemu-devel@nongnu.org; Wed, 18 Jan 2012 05:51:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RnT7D-0002qY-3Z for qemu-devel@nongnu.org; Wed, 18 Jan 2012 05:51:40 -0500 Received: from mail-gx0-f173.google.com ([209.85.161.173]:62572) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RnT7C-0002qT-Uw for qemu-devel@nongnu.org; Wed, 18 Jan 2012 05:51:39 -0500 Received: by ggnk1 with SMTP id k1so4666576ggn.4 for ; Wed, 18 Jan 2012 02:51:38 -0800 (PST) Sender: Paolo Bonzini Message-ID: <4F16A433.8000405@redhat.com> Date: Wed, 18 Jan 2012 11:51:31 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1326876486-13995-1-git-send-email-cyliu@suse.com> In-Reply-To: <1326876486-13995-1-git-send-email-cyliu@suse.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH][resend] Add -f option to qemu-nbd List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chunyan Liu Cc: stefanha@gmail.com, qemu-devel@nongnu.org On 01/18/2012 09:48 AM, Chunyan Liu wrote: > Stefan, could you help commit it if it's OK? Thanks. > Same as in thread: > http://lists.gnu.org/archive/html/qemu-devel/2011-12/msg01083.html > but rebase it to latest code. > > Add -f option to qemu-nbd to find a free nbd device for user and connect > disk image to that device. Hi Chunyan, I'm now the NBD maintainer so I can take care of the patch. There are still a couple of nits. The main problem is that the device name is never printed, which requires some imagination on part of the user. :) This pretty much requires that you set "verbose = 1" together with -f. This disables daemonization, unfortunately, but there isn't really any better way to do so. I'm inclined towards removing daemonization altogether from 1.1; other opinions are welcome. Anyhow, please set "verbose = 1" and change the help text for -c/-v from " -c, --connect=DEV connect FILE to the local NBD device DEV\n" " -v, --verbose display extra debugging information\n" to " -c, --connect=DEV connect FILE to the local NBD device DEV\n" " and run in the background\n" " -v, --verbose with -c, display extra information and\n" " do not run in the background\n" Since a respin is required, there are a couple of other comments inline. > syntax: qemu-nbd -f disk.img > --- > qemu-nbd.c | 76 ++++++++++++++++++++++++++++++++++++++++++----------------- > 1 files changed, 54 insertions(+), 22 deletions(-) > > diff --git a/qemu-nbd.c b/qemu-nbd.c > index eb61c33..f4c1437 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -33,7 +33,7 @@ > #include > #include > > -#define SOCKET_PATH "/var/lock/qemu-nbd-%s" > +#define SOCKET_PATH "/var/lock/qemu-nbd-%d" > > static NBDExport *exp; > static int verbose; > @@ -55,12 +55,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" > @@ -69,7 +70,7 @@ static void usage(const char *name) > " -V, --version output version information and exit\n" > "\n" > "Report bugs to\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; > @@ -213,9 +215,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; Just use errx(EXIT_FAILURE, "Could not find a free NBD device.") here. > + } > } > > /* update partition table */ > @@ -275,7 +310,7 @@ int main(int argc, char **argv) > const char *bindto = "0.0.0.0"; > int port = NBD_DEFAULT_PORT; > 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' }, > @@ -292,6 +327,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; > @@ -303,6 +339,7 @@ int main(int argc, char **argv) > int ret; > int fd; > int persistent = 0; > + int find = 0; > pthread_t client_thread; > > /* The client thread uses SIGTERM to interrupt the server. A signal > @@ -380,6 +417,9 @@ int main(int argc, char **argv) > case 'v': > verbose = 1; > break; > + case 'f': > + find = 1; > + break; > case 'V': > version(argv[0]); > exit(0); > @@ -414,7 +454,7 @@ int main(int argc, char **argv) > return 0; > } Please give an error here if "device && find" ("The -f option is incompatible with -c"). > - if (device&& !verbose) { > + if ((device || find)&& !verbose) { > int stderr_fd[2]; > pid_t pid; > int ret; > @@ -466,18 +506,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()); > } > } > > @@ -510,10 +542,10 @@ int main(int argc, char **argv) > 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); Just pass NULL here and test "device != NULL" in the nbd_client_thread. > if (ret != 0) { > errx(EXIT_FAILURE, "Failed to create client thread: %s", > strerror(ret)); > @@ -536,7 +568,7 @@ int main(int argc, char **argv) > unlink(sockpath); > } > > - if (device) { > + if (device || find) { > void *ret; > pthread_join(client_thread,&ret); > exit(ret != NULL); Thanks, Paolo