2012/1/18 Paolo Bonzini <pbonz...@redhat.com>:
> 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.
Not clear how it will use SOCKET_PATH and SOCKET_PATH_HELP.
SOCKET_PATH_HELP stores the abstract string of sock path? (e.g.
/var/lock/qemu-nbd-xxx)
>
>
>>> " -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...
>
>
> 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.
If nbd module is not loaded, open device will fail, we can add error info:
fd = open(device, O_RDWR);
if (fd == -1) {
fprintf(stderr, "Failed to open %s\n", device);
continue;
}
And in next place, if using err instead of fprintf, it will print ENOENT error
message. Is that enough?
>
>
>>> +
>>> + 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
>
