[Libguestfs] [PATCH nbdkit 4/4] Add floppy plugin.

[Richard W.M. Jones]

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