8:20 a.m.
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