From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:39057) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RnTez-0002Zg-Uo for qemu-devel@nongnu.org; Wed, 18 Jan 2012 06:26:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RnTey-0002Xw-8t for qemu-devel@nongnu.org; Wed, 18 Jan 2012 06:26:33 -0500 Received: from mx1.redhat.com ([209.132.183.28]:10035) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RnTex-0002Xj-SD for qemu-devel@nongnu.org; Wed, 18 Jan 2012 06:26:32 -0500 Message-ID: <4F16AC61.3000400@redhat.com> Date: Wed, 18 Jan 2012 12:26:25 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1326876486-13995-1-git-send-email-cyliu@suse.com> <4F16A56E.4040303@msgid.tls.msk.ru> In-Reply-To: <4F16A56E.4040303@msgid.tls.msk.ru> Content-Type: text/plain; charset=ISO-8859-1; 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: Michael Tokarev Cc: stefanha@gmail.com, Chunyan Liu , qemu-devel@nongnu.org On 01/18/2012 11:56 AM, Michael Tokarev wrote: > On 18.01.2012 12:48, 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. > > There's a (trivial) fix sent against qemu-nbd which will > make this patch to not apply again (the change of "fd" > variable in main() into devfd and sockfd). I dunno if > it is applied to any branch or not. Most active person > in this area appears to be Paolo Bonzini (Cc'd). I've not yet prepared a public NBD tree, but I will soon and I will apply that patch (strictly speaking I had a comment, but I can fix that up myself). >> @@ -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" > > This is a semantic change. Before, the socket was > named after the nbd device name, which, lacking the > -f option, was deterministic (given with -c option). > Now, since pid is random for the user, we don't have > a deterministic socket name anymore. I don't think > that using pid here makes any sense at all, especially > having in mind the double-fork the daemon is doing > at start. I think that we should continue using the > nbd device name here as before. That is not possible. The socket is needed by nbd_init, and you cannot know the name of the socket until you know the device name. The socket name is really an internal detail when using -c. > Note also that, while it is not currently supported > anyway, you're making this more difficult to change > the socket path by hardcoding it into two places. Yes, that's true. He could have two definitions (SOCKET_PATH with %d and SOCKET_PATH_HELP without) so that the occurrences would stay close in the source code. >> " -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; > > You also can use !device condition here instead of extra > variable: if device is set (non-NULL) use it, if it is not > set, find. This way there's no need to pass any args to > this function at all. But that's just my taste, nothing > more. > >> 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; >> + } > > Here, I'm not sure it should ignore all possible errors. > How about, say, EMFILE, or ENOENT, or lots of other possible > error conditions which may be reported here instead of > EBUSY? Not that it is very important again... Yes, though actually ENOENT is an important special case that is worth reporting to the user. It happens when the nbd module is not loaded. >> + >> + break; >> + } >> + >> + if (i>= max_nbd) { >> + fprintf(stderr, "Fail to find a free nbd device\n"); > > in all other places in qemu-nbd.c error reporting is done > differently, namely, using err() or errx() routine (or > warn()). The difference is important: err(3) also reports > strerror(errno) if errno is nonzero, so it will be clear > _which_ error happened. I understand that usage of err(3) > here is a bit more fragile since it is not a main thread. > Besides, it is "Failed" not "Fail". Indeed, I suggested using errx in my review but err is better because of ENOENT. Paolo