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