Libguestfs - Oct 2018 - Re: [PATCH nbdkit 4/4] Add floppy plugin.

On 10/28/18 5:13 AM, Richard W.M. Jones wrote:
> Create a virtual FAT-formatted floppy disk from a directory of files.
> 
> For example:
> 
>    nbdkit floppy /path/to/directory
> 
> The implementation of this is quite different from nbdkit-iso-plugin
> since we cannot use an external program.  Instead this plugin
> synthesizes the MBR partition and FAT32 structures that are required.
> 
> To do: Implement bootable virtual floppy using syslinux.  This is not
> straightforward as you have to either run the syslinux command on the
> virtual floppy, or else you have to synthesize the same set of
> operations that syslinux performs.  Also you have to deal with CHS
> geometry.
> ---
>   configure.ac                            |   2 +
>   plugins/floppy/Makefile.am              |  68 ++
>   plugins/floppy/directory-lfn.c          | 623 ++++++++++++++++++
>   plugins/floppy/floppy.c                 | 198 ++++++
>   plugins/floppy/nbdkit-floppy-plugin.pod |  87 +++
>   plugins/floppy/virtual-floppy.c         | 797 ++++++++++++++++++++++++
>   plugins/floppy/virtual-floppy.h         | 233 +++++++
>   plugins/iso/nbdkit-iso-plugin.pod       |   1 +
>   tests/Makefile.am                       |   6 +
>   tests/test-floppy.sh                    |  67 ++
>   10 files changed, 2082 insertions(+)
> 
> +/* Used for dealing with VFAT LFNs when creating a directory. */
> +struct lfn {
> +  const char *name;             /* Original Unix filename. */
> +  char short_base[8];           /* Short basename. */
> +  char short_ext[3];            /* Short file extension. */
> +  char *lfn;                    /* Long filename for MS-DOS as UTF16-LE.
*/
> +  size_t lfn_size;              /* Size *in bytes* of lfn. */
Including or excluding the trailing 2 bytes for a terminating NUL character?

> +/* Create the on disk directory table for dirs[di]. */
> +int
> +create_directory (size_t di, const char *label,
> +                  struct virtual_floppy *floppy)
> +{
> +  /* Add files. */
> +  attributes = DIR_ENTRY_ARCHIVE; /* Same as set by Linux kernel. */
> +  for (i = 0; i < nr_files; ++i) {
> +    const size_t fi = floppy->dirs[di].files[i];
> +    assert (fi < floppy->nr_files);
> +
> +    lfn = &lfns[nr_subdirs+i];
Spacing around +
> +/* Either truncate or pad a string (with spaces). */
> +void
> +pad_string (const char *label, size_t n, uint8_t *out)
> +{
> +  const size_t len = strlen (label);
> +
> +  memcpy (out, label, len <= n ? len : n);
> +  if (len < n)
> +    memset (out+len, ' ', n-len);
More operator spacing
> +}
> +
> +/* Add a directory entry to dirs[di].table. */
> +static int
> +add_directory_entry (const struct lfn *lfn,
> +                     uint8_t attributes, uint32_t file_size,
> +                     struct stat *statbuf,
> +                     size_t di, struct virtual_floppy *floppy)
> +{
> +  uint8_t seq, checksum;
> +  ssize_t i;
> +  size_t j;
> +  struct lfn_entry lfn_entry;
> +  struct dir_entry entry;
> +  int last_seq = 1;
> +
> +  /* Compute a checksum for the shortname.  In real LFN filesystems
> +   * this is used to check whether a non-LFN-aware operating system
> +   * (ie. MS-DOS) has edited the directory.
> +   */
> +  checksum = 0;
> +  for (j = 0; j < 8; ++j)
> +    checksum = ((checksum & 1) << 7) + (checksum >> 1) +
lfn->short_base[j];
> +  for (j = 0; j < 3; ++j)
> +    checksum = ((checksum & 1) << 7) + (checksum >> 1) +
lfn->short_ext[j];
> +
> +  /* LFN support.
> +   *
> +   * Iterate in reverse over the sequence numbers.  If the filename is:
> +   *
> +   *   "ABCDEFGHIJKLMNO"
> +   *
> +   * assuming those are UCS-2 codepoints, so lfn_size = 15*2 = 30,
> +   * then we generate these LFN sequences:
> +   *
> +   *   seq   byte_offset   s[13]
> +   *   0x42  26            "NO           "
> +   *   0x01  0             "ABCDEFGHIJKLM"
> +   */
> +  for (seq = 1 + lfn->lfn_size/2/13; seq >= 1; --seq) {
> +    size_t byte_offset = (seq-1)*2*13, r;
And again
> +    uint16_t s[13];
> +
> +    /* Copy the portion of the LFN into s.
> +     * r = Number of bytes from the original string to copy.
> +     */
> +    r = lfn->lfn_size - byte_offset;
> +    if (r > 26)
> +      memcpy (s, &lfn->lfn[byte_offset], 26);
> +    else {
> +      memcpy (s, &lfn->lfn[byte_offset], r);
> +      /* Pad remaining filename with 0. */
> +      for (j = r/2; j < 13; ++j)
And here
> +        s[j] = htole16 (0);
> +    }
> +
> +    memset (&lfn_entry, 0, sizeof lfn_entry);
> +    lfn_entry.seq = seq;
> +    if (last_seq) {
> +      lfn_entry.seq |= 0x40;
> +      last_seq = 0;
> +    }
> +    lfn_entry.attributes = 0xf;
> +    lfn_entry.checksum = checksum;
> +
> +    /* Copy the name portion to the fields in the LFN entry. */
> +    memcpy (lfn_entry.name1, &s[0], 5*2);
> +    memcpy (lfn_entry.name2, &s[5], 6*2);
> +    memcpy (lfn_entry.name3, &s[11], 2*2);
And again.
> +
> +    i = extend_dir_table (di, floppy);
> +    if (i == -1)
> +      return -1;
> +    memcpy (&floppy->dirs[di].table[i], &lfn_entry, sizeof
(struct dir_entry));
> +  }
> +
> +  /* Create the 8.3 (short name / DOS-compatible) entry. */
> +  memset (&entry, 0, sizeof entry);
> +  memcpy (entry.name, lfn->short_base, 8);
> +  memcpy (entry.name+8, lfn->short_ext, 3);
here too
> +  entry.attributes = attributes;
> +  set_times (statbuf, &entry);
> +  entry.size = htole32 (file_size);
> +  /* Note that entry.cluster_hi and .cluster_lo are set later on in
> +   * update_directory_first_cluster.
> +   */
> +
> +  i = extend_dir_table (di, floppy);
> +  if (i == -1)
> +    return -1;
> +  floppy->dirs[di].table[i] = entry;
> +
> +  return 0;
> +}
> +
> +static int
> +convert_long_file_names (struct lfn *lfns, size_t n)
> +{
> +
> +    /* Convert the original filename to UTF16-LE.  Maximum LFN length
> +     * is 0x3f * 13 = 819 UCS-2 characters.
> +     */
> +    if (convert_to_utf16le (lfn->name, &lfn->lfn,
&lfn->lfn_size) == -1)
> +      return -1;
> +    if (lfn->lfn_size > 2*819) {
> +      nbdkit_error ("%s: filename is too long", lfn->name);
And here. An error unlikely to happen since most file systems tend to 
limit things to 256 bytes, but don't remove the safety check.
> +      return -1;
> +    }
> +  }
> +
> +  /* Now we must see if some short filenames are duplicates and
> +   * rename them.  XXX Unfortunately O(n^2).
> +   */
> +  for (i = 1; i < n; ++i) {
> +    for (j = 0; j < i; ++j) {
> +      if (memcmp (lfns[i].short_base, lfns[j].short_base, 8) == 0
&&
> +          memcmp (lfns[i].short_ext, lfns[j].short_ext, 3) == 0) {
> +        char s[9];
> +        ssize_t k;
> +
> +        /* Entry i is a duplicate of j (j < i).  So we will rename i.
*/
> +        lfn = &lfns[i];
> +
> +        len = snprintf (s, sizeof s, "~%zu", i);
> +        assert (len >= 2 && len <= 8);
> +
> +        k = 8-len;
> +        while (k > 0 && lfn->short_base[k] == ' ')
> +          k--;
> +        memcpy (&lfn->short_base[k], s, len);
> +      }
> +    }
> +  }
Does this properly handle filenames with leading or trailing '.' but no 
other reason to rename a short name into LFN format?  (I didn't actually 
test, I just know that such filenames tend to have interesting effects 
on FAT).  Or was that an issue with FAT16, but not FAT32?

> +/* Extend dirs[di].table by 1 directory entry. */
> +static ssize_t
> +extend_dir_table (size_t di, struct virtual_floppy *floppy)
> +{
> +  struct dir_entry *p;
> +  size_t i;
> +
> +  i = floppy->dirs[di].table_entries;
> +  p = realloc (floppy->dirs[di].table, sizeof (struct dir_entry) *
(i+1));
More spacing
> +++ b/plugins/floppy/floppy.c
> +static int
> +floppy_config (const char *key, const char *value)
> +{
> +  if (strcmp (key, "dir") == 0) {
> +    dir = nbdkit_realpath (value);
> +    if (dir == NULL)
> +      return -1;
> +  }
Should you error if the user passes more than one dir= instead of 
leaking the earlier one?
> +++ b/plugins/floppy/nbdkit-floppy-plugin.pod
> @@ -0,0 +1,87 @@
> +=head1 NAME
> +
> +nbdkit-floppy-plugin - create virtual floppy disk from directory
> +
> +=head1 SYNOPSIS
> +
> + nbdkit floppy [dir=]DIRECTORY
> +               [label=LABEL]
> +
> +=head1 DESCRIPTION
> +
> +C<nbdkit-floppy-plugin> is a plugin for L<nbdkit(1)> which
creates a
> +virtual FAT-formatted floppy disk image from a directory on the fly.
> +The files in the specified directory (and subdirectories) appear in
> +the virtual floppy, which is served read-only over the NBD protocol.
> +
> +The virtual floppy disk will have a single partition (using an MBR
> +partition table).  In that partition will be a virtual FAT32
> +filesystem containing the files.  Long filenames are supported.
> +
> +=head1 EXAMPLE
> +
> +Create a virtual floppy disk:
> +
> + nbdkit floppy /path/to/directory
> +
> +=head1 PARAMETERS
> +
> +=over 4
> +
> +=item B<dir=>DIRECTORY
> +
> +Specify the directory containing files and subdirectories which will
> +be added to the virtual floppy disk.  Files inside this directory will
> +appear in the root directory of the virtual floppy.
> +
> +This parameter is required.
> +
> +In nbdkit E<ge> 1.8, C<dir=> may be omitted.  To ensure that
the
Do we still need the >= 1.8 disclaimer, given that this plugin is newer 
than the point where we added the feature? (I guess so, since you are 
still numbering things 1.7.x, and it's easier to rely on new features at 
the bump to 1.8)
> +directory name does not end up being parsed accidentally as
> +C<key=value>, prefix relative paths with C<./> (absolute paths
do not
> +need modification).
> +
> +=item B<label=>LABEL
> +
> +The optional volume label for the filesystem.  This may be up to 11
> +ASCII characters.  If omitted, C<NBDKITFLOPY> is used.
> +
> +=back
> +
> +=head1 LIMITATIONS
> +
> +The maximum size of the disk is around 2TB.  The maximum size of a
> +single file is 4GB.  Non-regular files (such as block special,
> +symbolic links, sockets) are not supported and will be ignored.
If I recall, there is also a limitation on the top-level directory 
having a maximum number of children (was it something like 256 or 512 
entries), since it does not get to continue on to secondary pages, 
unlike subdirectories.

Should symlinks pointing to somewhere within the directory be supported 
by expanding it into the contents visible through the symlink? (But if 
we ever add that, we have to be careful of symlink-to-directory loops, 
as well as race conditions where TOCTTOU could result in a symlink 
escaping the directory)
> +
> +The plugin does not support writes.
> +
> +The plugin does not save a temporary copy of the files, so you must
> +leave the directory alone while nbdkit is running, else you may get an
> +error for example if the plugin tries to open one of the files which
> +you have moved or deleted.  (This is different from how
> +L<nbdkit-iso-plugin(1)> works).
Is there anything we can do on Linux (such as mounting an overlayfs) to 
ensure that the user can't change the contents we are serving from 
underneath us?  But for now, I'm fine with the current restriction of 
just documenting that the user really shouldn't be modifying things 
behind nbdkit's back.
> +
> +The virtual floppy will not be bootable.  This could be added in
> +future (using SYSLINUX) but requires considerable work.  As a
> +workaround use L<nbdkit-iso-plugin(1)> instead.
> +
> +FAT32 is always used, even for small disks (where dosfstools, for
> +example, would choose FAT12 or FAT16).  This results in extra wasted
> +space, but since it is only I<virtual> wasted space it isn't
really
> +important, and it simplifies the implementation greatly.
> +
> +++ b/plugins/floppy/virtual-floppy.c
> +static ssize_t
> +visit (const char *dir, struct virtual_floppy *floppy)
> +{
> +  void *np;
> +  size_t di;
> +  char *origdir;
> +  DIR *DIR;
> +  struct dirent *d;
> +  int err;
> +  struct stat statbuf;
> +
> +  /* Allocate a new index in the directory array.  Note that the root
> +   * directory will always be at dirs[0].
> +   */
> +  di = floppy->nr_dirs;
> +  np = realloc (floppy->dirs, sizeof (struct dir) * (di+1));
More operator spacing.
> +  if (np == NULL) {
> +    nbdkit_error ("realloc: %m");
> +    goto error0;
> +  }
> +  floppy->dirs = np;
> +  floppy->nr_dirs++;
> +  memset (&floppy->dirs[di], 0, sizeof (struct dir));
> +
> +  /* Because this is called from config_complete, before nbdkit
> +   * daemonizes or starts any threads, it's safe to use chdir here and
> +   * greatly simplifies the code.  However we must chdir back to the
> +   * original directory at the end.
> +   */
> +  origdir = get_current_dir_name ();
> +  if (origdir == NULL) {
> +    nbdkit_error ("get_current_dir_name: %m");
> +    goto error0;
> +  }
> +  if (chdir (dir) == -1) {
> +    nbdkit_error ("chdir: %s: %m", dir);
> +    goto error1;
> +  }
> +
> +  DIR = opendir (".");
> +  if (DIR == NULL) {
> +    nbdkit_error ("opendir: %s: %m", dir);
> +    goto error1;
> +  }
> +
> +  errno = 0;
> +  while ((d = readdir (DIR)) != NULL) {
> +    if (strcmp (d->d_name, ".") == 0 ||
> +        strcmp (d->d_name, "..") == 0)
> +      continue;
> +
> +    if (lstat (d->d_name, &statbuf) == -1) {
> +      nbdkit_error ("stat: %s/%s: %m", dir, d->d_name);
> +      goto error2;
> +    }
> +
> +    /* Directory. */
> +    if (S_ISDIR (statbuf.st_mode)) {
> +      if (visit_subdirectory (dir, d->d_name, &statbuf, di, floppy)
== -1)
> +        goto error2;
> +    }
> +    /* Regular file. */
> +    else if (S_ISREG (statbuf.st_mode)) {
> +      if (visit_file (dir, d->d_name, &statbuf, di, floppy) == -1)
> +        goto error2;
> +    }
> +    /* else ALL other file types are ignored - see documentation. */
> +  }
Need to reset errno back to 0 before looping back to the next readdir() 
call.
> +
> +  /* Did readdir fail? */
> +  if (errno != 0) {
> +    nbdkit_error ("readdir: %s: %m", dir);
> +    goto error2;
> +  }
> +
> +  if (closedir (DIR) == -1) {
> +    nbdkit_error ("closedir: %s: %m", dir);
> +    goto error1;
> +  }
> +
> +  if (chdir (origdir) == -1) {
> +    nbdkit_error ("chdir: %s: %m", origdir);
> +    goto error0;
> +  }
> +  free (origdir);
> +  return di;
> +
> + error2:
> +  closedir (DIR);
> + error1:
> +  err = errno;
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wunused-result"
> +  chdir (origdir);
> +#pragma GCC diagnostic pop
> +  errno = err;
> +  free (origdir);
> + error0:
> +  return -1;
> +}
> +
> +/* This is called to visit a subdirectory in a directory.  It
> + * recursively calls the visit() function, and then adds the
> + * subdirectory to the list of subdirectories in the parent.
> + */
> +static int
> +visit_subdirectory (const char *dir, const char *name,
> +                    const struct stat *statbuf, size_t di,
> +                    struct virtual_floppy *floppy)
> +{
> +  void *np;
> +  char *subdir;
> +  ssize_t sdi;                  /* subdirectory index */
> +  size_t i;
> +
> +  if (asprintf (&subdir, "%s/%s", dir, name) == -1) {
> +    nbdkit_error ("asprintf: %m");
> +    return -1;
> +  }
> +  /* Recursively visit this directory.  As a side effect this adds the
> +   * new subdirectory to the global list of directories, and returns
> +   * the index in that list (sdi).
> +   */
> +  sdi = visit (subdir, floppy);
> +  if (sdi == -1) {
> +    free (subdir);
> +    return -1;
> +  }
> +  free (subdir);
> +
> +  /* We must set sdi->name because visit() cannot set it. */
> +  floppy->dirs[sdi].name = strdup (name);
> +  if (floppy->dirs[sdi].name == NULL) {
> +    nbdkit_error ("strdup: %m");
> +    return -1;
> +  }
> +  floppy->dirs[sdi].statbuf = *statbuf;
> +  floppy->dirs[sdi].pdi = di;
> +
> +  /* Add to the list of subdirs in the parent directory (di). */
> +  i = floppy->dirs[di].nr_subdirs;
> +  np = realloc (floppy->dirs[di].subdirs, sizeof (size_t) * (i+1));
more spacing
> +  if (np == NULL) {
> +    nbdkit_error ("realloc: %m");
> +    return -1;
> +  }
> +  floppy->dirs[di].subdirs = np;
> +  floppy->dirs[di].nr_subdirs++;
> +  floppy->dirs[di].subdirs[i] = sdi;
> +
> +  return 0;
> +}
> +
> +/* This is called to visit a file in a directory.  It performs some
> + * checks and then adds the file to the global list of files, and also
> + * adds the file to the list of files in the parent directory.
> + */
> +static int
> +visit_file (const char *dir, const char *name,
> +            const struct stat *statbuf, size_t di,
> +            struct virtual_floppy *floppy)
> +{
> +  void *np;
> +  char *host_path;
> +  size_t fi, i;
> +
> +  if (asprintf (&host_path, "%s/%s", dir, name) == -1) {
> +    nbdkit_error ("asprintf: %m");
> +    return -1;
> +  }
> +
> +  if (statbuf->st_size >= UINT32_MAX) {
> +    nbdkit_error ("%s: file is larger than maximum supported by
VFAT",
> +                  host_path);
> +    free (host_path);
> +    return -1;
> +  }
> +
> +  /* Add to global list of files. */
> +  fi = floppy->nr_files;
> +  np = realloc (floppy->files, sizeof (struct file) * (fi+1));
and again
> +  if (np == NULL) {
> +    nbdkit_error ("realloc: %m");
> +    free (host_path);
> +    return -1;
> +  }
> +  floppy->files = np;
> +  floppy->nr_files++;
> +  floppy->files[fi].name = strdup (name);
> +  if (floppy->files[fi].name == NULL) {
> +    nbdkit_error ("strdup: %m");
> +    free (host_path);
> +    return -1;
> +  }
> +  floppy->files[fi].host_path = host_path;
> +  floppy->files[fi].statbuf = *statbuf;
> +
> +  /* Add to the list of files in the parent directory (di). */
> +  i = floppy->dirs[di].nr_files;
> +  np = realloc (floppy->dirs[di].files, sizeof (size_t) * (i+1));
Recurring issue, isn't it :)

> +static int
> +create_regions (struct virtual_floppy *floppy)
> +{
> +  struct region region;
> +  size_t i;
> +
> +  /* MBR. */
> +  region.start = 0;
> +  region.end = SECTOR_SIZE-1;
and again

Overall, looks like a useful plugin!

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Tue, Oct 30, 2018 at 09:12:55AM -0500, Eric Blake
wrote:
> >+/* Used for dealing with VFAT LFNs when creating a directory. */
> >+struct lfn {
> >+  const char *name;             /* Original Unix filename. */
> >+  char short_base[8];           /* Short basename. */
> >+  char short_ext[3];            /* Short file extension. */
> >+  char *lfn;                    /* Long filename for MS-DOS as
UTF16-LE. */
> >+  size_t lfn_size;              /* Size *in bytes* of lfn. */
> 
> Including or excluding the trailing 2 bytes for a terminating NUL
character?
This is a very good question which I also asked and answered (by
looking at the Linux code).  The documentation says that there must be
a trailing 2-byte NUL, *and* then any further unused characters in the
LFN [recall they must be a multiple of 13 UCS-2 codepoints] are
0xFFFF.  However the Linux implementation doesn't care about either of
these things.  It's totally fine to end the string without any NUL
provided the end coincides with a 13 codepoint boundary.  Also to fill
the rest of the space with 0x0000.  So that's what this implementation
does.

Having said that I didn't check it works on Windows (or even MS-DOS!)
guests, which I really should do ...
> >+    lfn = &lfns[nr_subdirs+i];
> 
> Spacing around +
On the spacing issue I've tried not to be too religious here, and
instead group things logically.  For example:

  2*n + 1

would be fine.
> >+  /* Now we must see if some short filenames are duplicates and
> >+   * rename them.  XXX Unfortunately O(n^2).
> >+   */
> >+  for (i = 1; i < n; ++i) {
> >+    for (j = 0; j < i; ++j) {
> >+      if (memcmp (lfns[i].short_base, lfns[j].short_base, 8) == 0
&&
> >+          memcmp (lfns[i].short_ext, lfns[j].short_ext, 3) == 0) {
> >+        char s[9];
> >+        ssize_t k;
> >+
> >+        /* Entry i is a duplicate of j (j < i).  So we will rename
i. */
> >+        lfn = &lfns[i];
> >+
> >+        len = snprintf (s, sizeof s, "~%zu", i);
> >+        assert (len >= 2 && len <= 8);
> >+
> >+        k = 8-len;
> >+        while (k > 0 && lfn->short_base[k] == '
')
> >+          k--;
> >+        memcpy (&lfn->short_base[k], s, len);
> >+      }
> >+    }
> >+  }
> 
> Does this properly handle filenames with leading or trailing '.'
but
> no other reason to rename a short name into LFN format?  (I didn't
> actually test, I just know that such filenames tend to have
> interesting effects on FAT).  Or was that an issue with FAT16, but
> not FAT32?
So the current implementation differs from Windows in that it will
create an LFN for every name, even if the name is representable as a
short name.  For Linux this appears to have no ill effect.  Also dot
files work exactly as expected (in Linux).

What's interesting is that for real clients the short name is likely
never used (although I did test them by deleting the LFN code and
observing that Linux could still view the partition).

I don't believe that qemu can run whatever ancient MS-DOS predates
LFNs (like is it MS-DOS < 6?); or that there is another hypervisor
that can run ancient DOS and also supports NBD drives.
> >+static int
> >+floppy_config (const char *key, const char *value)
> >+{
> >+  if (strcmp (key, "dir") == 0) {
> >+    dir = nbdkit_realpath (value);
> >+    if (dir == NULL)
> >+      return -1;
> >+  }
> 
> Should you error if the user passes more than one dir= instead of
> leaking the earlier one?
Yes that's a bug, will fix.  The reason I didn't get this right
originally is because I was going to implement the same thing as
nbdkit-iso-plugin and have the multiple directories merge together,
but never got around to it.
> >+In nbdkit E<ge> 1.8, C<dir=> may be omitted.  To ensure
that the
> 
> Do we still need the >= 1.8 disclaimer, given that this plugin is
> newer than the point where we added the feature? (I guess so, since
> you are still numbering things 1.7.x, and it's easier to rely on new
> features at the bump to 1.8)
Yes, we don't really need it here.  I'd like to drop all these
disclaimers once 1.8 has been out for a while and all the distros have
updated.  I find this feature makes nbdkit very much more convenient.
> >+=head1 LIMITATIONS
> >+
> >+The maximum size of the disk is around 2TB.  The maximum size of a
> >+single file is 4GB.  Non-regular files (such as block special,
> >+symbolic links, sockets) are not supported and will be ignored.
> 
> If I recall, there is also a limitation on the top-level directory
> having a maximum number of children (was it something like 256 or
> 512 entries), since it does not get to continue on to secondary
> pages, unlike subdirectories.
Not in FAT32!  You are correct for FAT12/16 where there was limited
space in the root directory.
> Should symlinks pointing to somewhere within the directory be
> supported by expanding it into the contents visible through the
> symlink? (But if we ever add that, we have to be careful of
> symlink-to-directory loops, as well as race conditions where TOCTTOU
> could result in a symlink escaping the directory)
Exactly it's tricky to get it right, and ignoring them now doesn't
stop us from fixing it later if someone needs it.
> >+The plugin does not support writes.
> >+
> >+The plugin does not save a temporary copy of the files, so you must
> >+leave the directory alone while nbdkit is running, else you may get an
> >+error for example if the plugin tries to open one of the files which
> >+you have moved or deleted.  (This is different from how
> >+L<nbdkit-iso-plugin(1)> works).
> 
> Is there anything we can do on Linux (such as mounting an overlayfs)
> to ensure that the user can't change the contents we are serving
> from underneath us?  But for now, I'm fine with the current
> restriction of just documenting that the user really shouldn't be
> modifying things behind nbdkit's back.
I don't know, but would like to avoid needing to do anything hairy as
root.
> >+  errno = 0;
> >+  while ((d = readdir (DIR)) != NULL) {
> >+    if (strcmp (d->d_name, ".") == 0 ||
> >+        strcmp (d->d_name, "..") == 0)
> >+      continue;
> >+
> >+    if (lstat (d->d_name, &statbuf) == -1) {
> >+      nbdkit_error ("stat: %s/%s: %m", dir, d->d_name);
> >+      goto error2;
> >+    }
> >+
> >+    /* Directory. */
> >+    if (S_ISDIR (statbuf.st_mode)) {
> >+      if (visit_subdirectory (dir, d->d_name, &statbuf, di,
floppy) == -1)
> >+        goto error2;
> >+    }
> >+    /* Regular file. */
> >+    else if (S_ISREG (statbuf.st_mode)) {
> >+      if (visit_file (dir, d->d_name, &statbuf, di, floppy) ==
-1)
> >+        goto error2;
> >+    }
> >+    /* else ALL other file types are ignored - see documentation. */
> >+  }
> 
> Need to reset errno back to 0 before looping back to the next
> readdir() call.
Hmm, really?  If so, this is a class of bugs we have made throughout
libguestfs code.  Why would errno be set in the loop?
> Overall, looks like a useful plugin!
Thanks for the detailed review!

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On 10/30/18 11:42 AM, Richard W.M. Jones wrote:
> 
>>> +  errno = 0;
>>> +  while ((d = readdir (DIR)) != NULL) {
>>> +    if (strcmp (d->d_name, ".") == 0 ||
>>> +        strcmp (d->d_name, "..") == 0)
>>> +      continue;
strcmp() leaves errno alone (well, POSIX doesn't guarantee that, but no 
sane implementation of strcmp() would change errno during a string compare)
>>> +
>>> +    if (lstat (d->d_name, &statbuf) == -1) {
>>> +      nbdkit_error ("stat: %s/%s: %m", dir,
d->d_name);
>>> +      goto error2;
>>> +    }
However, errno is undefined if lstat() succeeds. If it fails, you don't 
call readdir() again; but if it succeeds, you cannot rely on libc having 
left errno == 0.
>>> +
>>> +    /* Directory. */
>>> +    if (S_ISDIR (statbuf.st_mode)) {
>>> +      if (visit_subdirectory (dir, d->d_name, &statbuf, di,
floppy) == -1)
>>> +        goto error2;
>>> +    }
And then you have to audit whether your visit_subdirectory() code left 
errno unchanged...
>>> +    /* Regular file. */
>>> +    else if (S_ISREG (statbuf.st_mode)) {
>>> +      if (visit_file (dir, d->d_name, &statbuf, di, floppy)
== -1)
>>> +        goto error2;
>>> +    }
...and visit_file().  Looking at just visit_file():
> +static int
> +visit_file (const char *dir, const char *name,
> +            const struct stat *statbuf, size_t di,
> +            struct virtual_floppy *floppy)
> +{
> +  void *np;
> +  char *host_path;
> +  size_t fi, i;
> +
> +  if (asprintf (&host_path, "%s/%s", dir, name) == -1) {
> +    nbdkit_error ("asprintf: %m");
> +    return -1;
> +  }
asprintf() may have modified errno...
> +  /* Add to global list of files. */
> +  fi = floppy->nr_files;
> +  np = realloc (floppy->files, sizeof (struct file) * (fi+1));
Likewise a successful realloc()...
> +  if (np == NULL) {
> +    nbdkit_error ("realloc: %m");
> +    free (host_path);
> +    return -1;
> +  }
> +  floppy->files = np;
> +  floppy->nr_files++;
> +  floppy->files[fi].name = strdup (name);
even a successful strdup(). Etc.

In short, POSIX guarantees that errno is set on failure, but leaves 
errno undefined on success after many library functions (there are a 
few, like free(), where I have been working to get POSIX to explicitly 
state that errno is left unchanged - but those are mainly functions you 
use on cleanup paths, where it is easier to program if you can guarantee 
that your cleanup doesn't corrupt the original error).
>> Need to reset errno back to 0 before looping back to the next
>> readdir() call.
> 
> Hmm, really?  If so, this is a class of bugs we have made throughout
> libguestfs code.
Yes, this is a common bug, and it looks like libguestfs needs an audit 
fixing it.  The only SAFE way to call readdir() is to immediately set 
errno = 0 just beforehand, on every iteration (not just the first).
>  Why would errno be set in the loop?
Because you called into libc.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org