Re: [Qemu-devel] [PATCH v4 2/3] Extract code to nbd_setup function to be used for many purposes

[Stefan Hajnoczi]

On Wed, Dec 7, 2011 at 4:23 AM, Chunyan Liu <address@hidden> wrote:

Overall looks good, some suggestions:

> @@ -53,7 +53,7 @@ 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-%s)\n"
>  "  -r, --read-only      export read-only\n"
>  "  -P, --partition=NUM  only expose partition NUM\n"
>  "  -s, --snapshot       use snapshot file\n"
> @@ -67,7 +67,7 @@ static void usage(const char *name)
>  "  -V, --version        output version information and exit\n"
>  "\n"
>  "Report bugs to <address@hidden>\n"
> -    , name, NBD_DEFAULT_PORT, "DEVICE");
> +    , name, NBD_DEFAULT_PORT, "PID");
>  }

Since we're not using the SOCKET_PATH format string anymore it's nicer
to drop this format string argument and just change to "(default
/var/lock/qemu-nbd-PID" above.

> +        fd = open(device, O_RDWR);
> +        if (fd == -1) {
> +            fprintf(stderr, "Failed to open %s", device);

Missing \n in string.

> +            goto out;
> +        }
> +
> +        ret = nbd_init(fd, sock, nbdflags, size, blocksize);
> +        if (ret == -1) {
> +            goto out;
> +        }
> +    } else {
> +        int i = 0;
> +        int max_nbd = 16;
> +        for (i = 0; i < max_nbd; i++) {
> +            if (!asprintf(&device, "/dev/nbd%d", i)) {

asprintf() is GNU and BSD only (not C or POSIX).  I suggest using
g_strdup_printf() instead which is guaranteed to be available because
QEMU builds against glib:

http://developer.gnome.org/glib/2.28/glib-String-Utility-Functions.html#g-strdup-printf

> +                continue;
> +            }
>
> +
> +            fd = open(device, O_RDWR);
> +            if (fd == -1 || nbd_init(fd, sock, nbdflags, size, blocksize)
> == -1) {

nbd_init() does not close(fd) on failure.  Please separate the open()
and nbd_init() error cases and close the fd.

Stefan