[Libguestfs] [nbdkit PATCH 2/4] file: Add .list_exports support

[Eric Blake]

On 8/7/20 6:57 AM, Richard W.M. Jones wrote:
> On Thu, Aug 06, 2020 at 09:23:46PM -0500, Eric Blake wrote:
>> +  if (!filename == !directory) {
> 
> A bit tricksy.  In plugins/nbd/nbd.c I used:
> 
>    int c = !!sockname + !!hostname + !!uri +
>      (command.size > 0) + (socket_fd >= 0) + !!raw_cid;
> 
>    /* Check the user passed exactly one connection parameter. */
>    if (c > 1) {
>      nbdkit_error ("cannot mix Unix ‘socket’, TCP ‘hostname’/‘port’, ‘vsock’, "
>                    "‘command’, ‘socket-fd’ and ‘uri’ parameters");
>      return -1;
>    }
>    if (c == 0) {
>      nbdkit_error ("exactly one of ‘socket’, ‘hostname’, ‘vsock’, ‘command’, "
>                    "‘socket-fd’ and ‘uri’ parameters must be specified");
>      return -1;
>    }
> 
> which might be longer but a bit clearer?

Yes, having two separate error messages is more verbose in the code, but 
more friendly to the user.  I will adjust the code.

> 
>> +  if (directory && (dir = opendir (directory)) == NULL) {
>> +    nbdkit_error ("opendir: %m");
>>       return -1;
>>     }
> 
> What happens if the contents of the directory change while the file
> plugin is running?  I guess we're using rewinddir so that's fine.

Each client that calls NBD_OPT_LIST sees the current contents of the 
directory.  Hmm - the test I wrote fires up a new nbdkit per client; but 
it might be interesting to also have a test that runs with a single 
long-running nbdkit showing that we do track live content changes.

> 
> But maybe we want to defer opening this until we actually do list_exports?
> 
> I can see arguments both ways TBH, one obvious one being that it'd be
> nice to fail early if directory doesn't exist at all.  However if we
> defer opening the directory til it is needed then we would have to use
> nbdkit_realpath on the directory name, and that should check that the
> directory (or something) exists.

Right now, I had .config use nbdkit_realpath(), which fails if it 
doesn't exist; I'd have to use nbdkit_absolute_path() instead if we want 
to defer the diropen() to the last possible moment.

If we defer, what happens when the directory still doesn't exist? 
NBD_OPT_LIST fails (nothing to list) - it could either fail nicely 
(return 0, which lists nothing) or fail hard (return -1, which tells the 
client that NBD_OPT_LIST is not available), but I don't see any real 
reason to favor one approach over the other.  Likewise, .open will fail 
(an export within a missing directory is impossible).  But the worst 
that happens is the client can't connect; you then create the directory, 
and the next client will succeed.  Okay, maybe that's an argument for 
deferral.  I'll play with it.


>>
>> -  h->fd = open (filename, flags);
>> +  h->fd = open (h->file, flags);
> 
> Maybe openat is safer than trying to concatenate filenames?

Ah, nice. But openat requires an fd() - which we get if we open the DIR* 
early.  That is, to use openat(), we need our opendir() early, rather 
than deferred.

> 
> I would say that Windows is part of the argument here, but we don't
> support it, and Windows doesn't have openat.  Also there's an argument
> that you want the full filename for error messages, but do we in fact
> want to echo the exportname (client controlled) to log files?

Windows actually does sort-of support openat semantics (the Cygwin folks 
were able to implement openat() on top of windows native code), but you 
are correct that it does not yet have actual openat() in libc.  But, as 
you say, if someone actually cares enough to port nbdkit to Windows, 
they can address that as part of their porting effort.

As for the error messages, the command line knows what the directory is, 
so logging _just_ the exportname is probably sufficient (logging a 
concatenated name is just extra work).

> 
>> +# Hack: This rejects libnbd 1.3.9, but we really need libnbd >= 1.3.11,
>> +# which does not have its own decent witness...
>> +requires nbdsh -c 'print (h.get_list_export_description)'
> 
> Some variation on:
> 
> $ nbdsh -c 'print(h.get_version())'
> 1.3.11
> $ nbdsh -c 'from packaging import version ; print(version.parse(h.get_version()) >= version.parse("1.3.11"))'
> True
> 
> However 1.3.10 was only present for a short time and I already got rid
> of it in Rawhide because it affected other nbdkit tests, so probably
> what you have already is fine in practice.

As it is, I'm still trying to work on my libnbd patches that make it 
possible to do NBD_OPT_LIST followed by NBD_OPT_GO(name) on a single 
connection, which may get rid of h.get_list_export_description (by 
replacing it with a callback to the nbd_opt_list call), so this whole 
area is in flux.  We'll clean it up prior to releasing either libnbd 1.4 
or nbdkit 1.22.  That window with broken 'nbdinfo --json --list' on 
multiple exports was annoying, but if it is short-lived, we shouldn't 
need a long-term hack in our testsuite.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org