11:42 a.m.
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