On Wed, Dec 7, 2011 at 4:23 AM, Chunyan Liu <cy...@suse.com> 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 <anth...@codemonkey.ws>\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
