Laszlo Ersek
2022-Aug-18 08:27 UTC
[Libguestfs] [PATCH nbdkit 1/2] file: Add an internal "mode"
On 08/17/22 17:38, Richard W.M. Jones wrote:> Previously we relied on the implicit assumption filename xor directory, > representing two modes. Make this explicit with an internal mode > variable. > > This is just refactoring and should not change the functionality. > However we're now more strict about duplicate file= or dir= parameters > appearing on the command line. Previously only the last one had an > effect and the others were silently ignored. Now we give an error in > this case. eg this worked before but now gives an error: > > $ ./nbdkit file /var/tmp /var/tmp/fedora-36.img > nbdkit: error: file|dir parameter can only appear once on the command line > --- > plugins/file/file.c | 124 +++++++++++++++++++++++++++----------------- > 1 file changed, 76 insertions(+), 48 deletions(-) > > diff --git a/plugins/file/file.c b/plugins/file/file.c > index aefca9ee2..f673ec132 100644 > --- a/plugins/file/file.c > +++ b/plugins/file/file.c > @@ -70,6 +70,7 @@ > #include "isaligned.h" > #include "fdatasync.h" > > +static enum { mode_none, mode_filename, mode_directory } mode = mode_none; > static char *filename = NULL; > static char *directory = NULL; > > @@ -180,14 +181,23 @@ file_config (const char *key, const char *value) > * existence checks to the last possible moment. > */ > if (strcmp (key, "file") == 0) { > - free (filename); > + if (mode != mode_none) { > + wrong_mode:*shudder* please move error handling sections to the end of the function. It's OK to jump forward in exceptional cases, but crossing jump lines (i.e. improper balancing / nesting) and backward jumps for error handling are terrible. Just my opinion :) Thanks Laszlo> + nbdkit_error ("%s parameter can only appear once on the command line", > + "file|dir"); > + return -1; > + } > + mode = mode_filename; > + assert (filename == NULL); > filename = nbdkit_realpath (value); > if (!filename) > return -1; > } > else if (strcmp (key, "directory") == 0 || > strcmp (key, "dir") == 0) { > - free (directory); > + if (mode != mode_none) goto wrong_mode; > + mode = mode_directory; > + assert (directory == NULL); > directory = nbdkit_realpath (value); > if (!directory) > return -1; > @@ -252,22 +262,27 @@ file_config_complete (void) > int r; > struct stat sb; > > - if (!filename && !directory) { > + if (mode == mode_none) { > nbdkit_error ("you must supply either [file=]<FILENAME> or " > "dir=<DIRNAME> parameter after the plugin name " > "on the command line"); > return -1; > } > - if (filename && directory) { > - nbdkit_error ("file= and dir= cannot be used at the same time"); > - return -1; > + if (mode == mode_filename) { > + assert (filename != NULL); > + assert (directory == NULL); > + } > + if (mode == mode_directory) { > + assert (filename == NULL); > + assert (directory != NULL); > } > > /* Sanity check now, rather than waiting for first client open. > * See also comment in .config about use of nbdkit_realpath. > * Yes, this is a harmless TOCTTOU race. > */ > - if (filename) { > + switch (mode) { > + case mode_filename: > r = stat (filename, &sb); > if (r == 0 && S_ISDIR (sb.st_mode)) { > nbdkit_error ("use dir= to serve files within %s", filename); > @@ -277,10 +292,14 @@ file_config_complete (void) > nbdkit_error ("file is not regular or block device: %s", filename); > return -1; > } > - } > - else if (stat (directory, &sb) == -1 || !S_ISDIR (sb.st_mode)) { > - nbdkit_error ("expecting a directory: %s", directory); > - return -1; > + break; > + case mode_directory: > + if (stat (directory, &sb) == -1 || !S_ISDIR (sb.st_mode)) { > + nbdkit_error ("expecting a directory: %s", directory); > + return -1; > + } > + break; > + default: abort (); > } > > return 0; > @@ -320,46 +339,51 @@ file_list_exports (int readonly, int default_only, > struct stat sb; > int fd; > > - if (!directory) > + switch (mode) { > + case mode_filename: > return nbdkit_add_export (exports, "", NULL); > > - dir = opendir (directory); > - if (dir == NULL) { > - nbdkit_error ("opendir: %m"); > - return -1; > - } > - fd = dirfd (dir); > - if (fd == -1) { > - nbdkit_error ("dirfd: %m"); > - closedir (dir); > - return -1; > - } > - errno = 0; > - while ((entry = readdir (dir)) != NULL) { > - int r = -1; > -#if HAVE_STRUCT_DIRENT_D_TYPE > - if (entry->d_type == DT_BLK || entry->d_type == DT_REG) > - r = 1; > - else if (entry->d_type != DT_LNK && entry->d_type != DT_UNKNOWN) > - r = 0; > -#endif > - /* TODO: when chasing symlinks, is statx any nicer than fstatat? */ > - if (r < 0 && fstatat (fd, entry->d_name, &sb, 0) == 0 && > - (S_ISREG (sb.st_mode) || S_ISBLK (sb.st_mode))) > - r = 1; > - if (r == 1 && nbdkit_add_export (exports, entry->d_name, NULL) == -1) { > + case mode_directory: > + dir = opendir (directory); > + if (dir == NULL) { > + nbdkit_error ("opendir: %m"); > + return -1; > + } > + fd = dirfd (dir); > + if (fd == -1) { > + nbdkit_error ("dirfd: %m"); > closedir (dir); > return -1; > } > errno = 0; > - } > - if (errno) { > - nbdkit_error ("readdir: %m"); > + while ((entry = readdir (dir)) != NULL) { > + int r = -1; > +#if HAVE_STRUCT_DIRENT_D_TYPE > + if (entry->d_type == DT_BLK || entry->d_type == DT_REG) > + r = 1; > + else if (entry->d_type != DT_LNK && entry->d_type != DT_UNKNOWN) > + r = 0; > +#endif > + /* TODO: when chasing symlinks, is statx any nicer than fstatat? */ > + if (r < 0 && fstatat (fd, entry->d_name, &sb, 0) == 0 && > + (S_ISREG (sb.st_mode) || S_ISBLK (sb.st_mode))) > + r = 1; > + if (r == 1 && nbdkit_add_export (exports, entry->d_name, NULL) == -1) { > + closedir (dir); > + return -1; > + } > + errno = 0; > + } > + if (errno) { > + nbdkit_error ("readdir: %m"); > + closedir (dir); > + return -1; > + } > closedir (dir); > - return -1; > + return 0; > + > + default: abort (); > } > - closedir (dir); > - return 0; > } > > /* The per-connection handle. */ > @@ -384,7 +408,8 @@ file_open (int readonly) > const char *file; > int dfd = -1; > > - if (directory) { > + switch (mode) { > + case mode_directory: > file = nbdkit_export_name (); > if (strchr (file, '/')) { > nbdkit_error ("exportname cannot contain /"); > @@ -396,9 +421,12 @@ file_open (int readonly) > nbdkit_error ("open %s: %m", directory); > return NULL; > } > - } > - else > + break; > + case mode_filename: > file = filename; > + break; > + default: abort (); > + } > > h = malloc (sizeof *h); > if (h == NULL) { > @@ -680,9 +708,9 @@ file_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset, > > #if defined (FALLOC_FL_PUNCH_HOLE) || defined (FALLOC_FL_ZERO_RANGE) > static int > -do_fallocate (int fd, int mode, off_t offset, off_t len) > +do_fallocate (int fd, int mode_, off_t offset, off_t len) > { > - int r = fallocate (fd, mode, offset, len); > + int r = fallocate (fd, mode_, offset, len); > if (r == -1 && errno == ENODEV) { > /* kernel 3.10 fails with ENODEV for block device. Kernel >= 4.9 fails > with EOPNOTSUPP in this case. Normalize errno to simplify callers. */ >
Richard W.M. Jones
2022-Aug-18 09:32 UTC
[Libguestfs] [PATCH nbdkit 1/2] file: Add an internal "mode"
On Thu, Aug 18, 2022 at 10:27:11AM +0200, Laszlo Ersek wrote:> On 08/17/22 17:38, Richard W.M. Jones wrote: > > Previously we relied on the implicit assumption filename xor directory, > > representing two modes. Make this explicit with an internal mode > > variable. > > > > This is just refactoring and should not change the functionality. > > However we're now more strict about duplicate file= or dir= parameters > > appearing on the command line. Previously only the last one had an > > effect and the others were silently ignored. Now we give an error in > > this case. eg this worked before but now gives an error: > > > > $ ./nbdkit file /var/tmp /var/tmp/fedora-36.img > > nbdkit: error: file|dir parameter can only appear once on the command line > > --- > > plugins/file/file.c | 124 +++++++++++++++++++++++++++----------------- > > 1 file changed, 76 insertions(+), 48 deletions(-) > > > > diff --git a/plugins/file/file.c b/plugins/file/file.c > > index aefca9ee2..f673ec132 100644 > > --- a/plugins/file/file.c > > +++ b/plugins/file/file.c > > @@ -70,6 +70,7 @@ > > #include "isaligned.h" > > #include "fdatasync.h" > > > > +static enum { mode_none, mode_filename, mode_directory } mode = mode_none; > > static char *filename = NULL; > > static char *directory = NULL; > > > > @@ -180,14 +181,23 @@ file_config (const char *key, const char *value) > > * existence checks to the last possible moment. > > */ > > if (strcmp (key, "file") == 0) { > > - free (filename); > > + if (mode != mode_none) { > > + wrong_mode: > > *shudder* > > please move error handling sections to the end of the function. It's OK > to jump forward in exceptional cases, but crossing jump lines (i.e. > improper balancing / nesting) and backward jumps for error handling are > terrible.I don't think I really have a problem with it, but I moved it to the end of the function. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
Richard W.M. Jones
2022-Aug-18 09:53 UTC
[Libguestfs] [PATCH nbdkit 1/2] file: Add an internal "mode"
This is upstream in 5 commits 5764fa6f4..dd28b0054: https://gitlab.com/nbdkit/nbdkit/-/commit/2620d95a4585204f0db0d55c0d41276d11970436 https://gitlab.com/nbdkit/nbdkit/-/commit/185e7d4010b353f36b5ca5d47467a770c530e58c https://gitlab.com/nbdkit/nbdkit/-/commit/17655b1ca67caa454e7a4ac83bc8c052b79a692d https://gitlab.com/nbdkit/nbdkit/-/commit/e7fdffde8142fb083625678b7a55455751185502 https://gitlab.com/nbdkit/nbdkit/-/commit/dd28b005430d020ccd1825437937c317332d3007 This also includes dirfd functionality and a rather complicated test for that. Turns out that bash refuses to open a directory as a file descriptor :-( 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
Eric Blake
2022-Aug-18 14:35 UTC
[Libguestfs] [PATCH nbdkit 1/2] file: Add an internal "mode"
On Thu, Aug 18, 2022 at 10:27:11AM +0200, Laszlo Ersek wrote:> > @@ -180,14 +181,23 @@ file_config (const char *key, const char *value) > > * existence checks to the last possible moment. > > */ > > if (strcmp (key, "file") == 0) { > > - free (filename); > > + if (mode != mode_none) { > > + wrong_mode: > > *shudder* > > please move error handling sections to the end of the function. It's OK > to jump forward in exceptional cases, but crossing jump lines (i.e. > improper balancing / nesting) and backward jumps for error handling are > terrible. > > Just my opinion :)I've used the same style in recent patches to libnbd (because it was pre-existing), but I can also agree to favor a forward jump to a common error handling at the bottom of the function rather than a backward jump to a mid-if label. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org