Re: [Qemu-devel] [PATCH][resend] Add -f option to qemu-nbd

[Chunyan Liu]

2012/1/18 Paolo Bonzini <address@hidden>:
> 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<address@hidden>\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
>