Richard W.M. Jones
2022-Aug-17 15:38 UTC
[Libguestfs] [PATCH nbdkit 1/2] file: Add an internal "mode"
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:
+ 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. */
--
2.37.0.rc2
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. */ >