Re: [PATCH] utils/mount: canonicalize mountpoints

[Gabriele Giacone]

On Sun, Jun 15, 2014 at 8:32 AM, Samuel Thibault
<samuel.thibault@gnu.org> wrote:
> Gabriele Giacone, le Sun 15 Jun 2014 02:23:49 +0200, a écrit :
>> On Sun, Jun 15, 2014 at 1:59 AM, Samuel Thibault
>> <samuel.thibault@gnu.org> wrote:
>> I thought it was already detailed enough: first and last hunks useless
>> if patch is applied are:
>
> That's only the goal. How does canonicalizing the paths actually
> fix that?  The problem is that canonicalizing paths will have other
> consequences, so we need to be convinced that it is the proper way to
> fix the goal. It's the explanation you provided below that is needed.

Which consequences? To me it just canonicalizes paths i.e. "realpath()
expands all symbolic links and resolves references to /./, /../ and
extra '/' characters in the null-terminated string named by path to
produce a canonicalized absolute pathname. [...] The resulting path
will have no symbolic link, /./ or /../ components."

>> Such hunks are workarounds at the moment because hurd mounts paths by
>> keeping superfluous '/' and '.' in pathnames if specified at mount
>> time, which need to be specified at umount time as well to umount
>> successfully.
>
> But then that won't fix the case when people use settrans by hand, so
> perhaps it should be rather done there?

Yes, that should fix all cases.

> Actually, I don't understand why canonicalizing the mountpoint in mount
> would be needed, does it really fix something?  Is the mountpoint, as
> provided on the mount command line, really recorded somwhere?

It is, no idea where. Try to mount something under a mountpoint with
superfluous '.' and '/' like e.g. /mnt/./test0//test1// then try to
umount it. You'll have to specify exactly what you specified at
mount(/settrans) time. That's what it fixes.

> Isn't it the device part (in the bind case, the source directory) which would
> need a canonicalization?  Then, shouldn't it be perhaps firmlink which
> should canonicalize its source directory?

It also affects device names, which are translators. Fixing settrans
should fix them too.
Try to mount /dev/./hd42 for instance, proc/mounts will remember
device name as-is and you'll have to pass /dev/./hd42 to umount to
unmount by device name (that would uncleanly umount it BTW).

In the bind case proc/mounts says source directory is device source
directory resides on (as on linux).

>> Patch simply makes mount/umount utilities fix through realpath
>> mountpoints/device pathnames to pass to settrans, making workarounds
>> above useless.
>
> That's a way to fix it, yes, but is it really the proper way?  It has
> other consequences on their semantic, so we have to weigh that.

I still can't see consequences about this mount/umount patch.
Fixing settrans would probably be the best way and more invasive but
I'm missing consequences in this case too. realpath following links it
should not? any case paths must keep extra '/' and '.'?


-- 
G..e