mountd: Allow spaces in directory paths

[dan.mcgregor_usask.ca]

Summary

The previous code would correctly parse strings including quotation
marks (") or backslash (/), but the tests when creating the export
includes them in the final string. This prevents exporting paths
with embedded spaces, for example "/exports/with space". Trying
results in log lines resembling:

mountd[1337]: bad exports list line '/exports/with\ space': /exports/with\ space: lstat() failed: No such file or directory.

Turns out that when creating its exports list, zfs escapes strings
in a format compatible with vis(3). Since I expect that zfs sharenfs
is the dominating use case for generating an exports list, use
strunvis(3) to parse the export path. The result is lines like the
following allow spaces:

/exports/with\040space -network 192.168.0 -mask 255.255.255.0

Manually created exports lists can be created with vis(1):

$ echo -n '/exports/with space' | vis -M
/exports/with\040space

[dan.mcgregor_usask.ca]

Hm. Missed updating the documentation. I'll update that too.

[dan.mcgregor_usask.ca]

Updated the exports(5) manual page.

[dan.mcgregor_usask.ca]

Include example export entry with spaces

[rmacklem]

I have added kevans as a reviewer. Basically, I would like to
check that ZFS does and intends to keep doing the "vis"
compatible encoding of blanks, etc in directory names.

If so the patch looks reasonable at a glance, but I need
to take a closer look (and also see if any changes are needed
for the exports.db case).
(I am completely unfamiliar with what "sharenfs: generates.)

[rmacklem]

Ok, so I see what nfs_escape_mountpoint() in the ZFS code currently
does. It looks like it is "vis" compatible, although I am going to do
a quick test with all the "\t\n\v\f\r \\" chars in the original.
If that test works, I'll take a close look at the patch.

Thanks for the submission, rick

[rmacklem]

Have you tested the case where there is a single space in
the directory name?

I think nextfield() will see the "\" and then scan for another
one. (I don't know why both \ and " are supported for
escapes, but to make this work, I think we would need to
make netxfield() only use " as escape?
(I notice man exports doesn't even mention the quotations
and I am not sure where they were meant to be used?)

usr.sbin/mountd/mountd.c

1571

Although this looks correct, I think

unvis_dir[0] = '\0';

is more readable.

1638

I think unvis_len should be checked for >= 0
(or != -1), since that is what is returned if
strnunvis() fails.
Something like:

if (unvis_len < 0) {
    getexp_err(ep, tgrp, "Cannot strunvis decode dir");
    goto nextline;
}

[rmacklem]

Oops, I take my last comment w.r.t. a
single space back. I now see that nextfield()
is handling a single or double quote mark.
(I misread the single quote mark as using a
\ for quotation.)

So, the only thing I see in the code is checking
unvis_len for -1.

[dan.mcgregor_usask.ca]

usr.sbin/mountd/mountd.c

1638

Makes sense. I'll update this review in the next hour or so.

[dan.mcgregor_usask.ca]

Added requested strnvis error checking.

[dan.mcgregor_usask.ca]

usr.sbin/mountd/mountd.c

1571

Sure, I could also initialize it on line 1565, but that doesn't seem to match the existing style of this file. Both do the same thing, but readability is important.

[rmacklem]

It looks ok to me now. Do you have a commit bit?
(If not, I can commit it.)

I think the man page update might need a little
more work, but that can be done separately.
(I am thinking that the cases of "blank" and "tab"
replacement should be explicit, since those are
what need to be done to make nextfield() work.)

[dan.mcgregor_usask.ca]

In D42432#968666, @rmacklem wrote:

It looks ok to me now. Do you have a commit bit?
(If not, I can commit it.)

I do not, so that'd be appreciated.

I think the man page update might need a little
more work, but that can be done separately.
(I am thinking that the cases of "blank" and "tab"
replacement should be explicit, since those are
what need to be done to make nextfield() work.)

That's fair. The itch I was scratching was embedded spaces, so that's the example I wrote, but the other special characters could cause confusing issues too.