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).
See comments inline below.
> Add -f option to qemu-nbd to find a free nbd device for user and connect
> disk image to that device.
> 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 <libgen.h>
> #include <pthread.h>
>
> -#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"
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.
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.
(It may need change due to /var/lock => /run/lock
transition but old /var/lock/ will continue to work
anyway, since it gets converted to a symlink pointing
to /run/lock).
> " -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 <anth...@codemonkey.ws>\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...
> +
> + 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".
Thanks,
/mjt
