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?
+ 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. 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.
+ if (directory) { + const char *exportname = nbdkit_export_name (); + + if (strchr (exportname, '/')) { + nbdkit_error ("exportname cannot contain /"); + errno = EINVAL; + free (h); + return NULL; + } + if (asprintf (&h->file, "%s/%s", directory, exportname) == -1) { + nbdkit_error ("asprintf: %m"); + free (h); + return NULL; + } + } + else + h->file = strdup (filename); + if (h->file == NULL) { + nbdkit_error ("malloc: %m"); + free (h); + return NULL; + } + flags = O_CLOEXEC|O_NOCTTY; if (readonly) flags |= O_RDONLY; else flags |= O_RDWR;
- h->fd = open (filename, flags); + h->fd = open (h->file, flags);
Maybe openat is safer than trying to concatenate filenames? 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?
+# 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. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top