Re: [PATCH] xfsprogs: ensure growfs rejects non-existent mount point

[Eric Sandeen <sandeen@sandeen.net>]

On 4/7/17 12:58 PM, Bill O'Donnell wrote:
> xfs_growfs manpage clearly states that the filesystem must
> be mounted to be grown. Current behavior allows xfs_growfs
> to proceed if the filesystem /containing/ the path
> of the desired target is mounted. This is not the specified
> behavior. Instead, also check the targeted fs argument against
> the entry found in the fstable lookup. Unless the targeted
> fs is actually mounted, reject the command.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
>  growfs/xfs_growfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
> index a294e14..05630b8 100644
> --- a/growfs/xfs_growfs.c
> +++ b/growfs/xfs_growfs.c
> @@ -203,7 +203,7 @@ main(int argc, char **argv)
>  
>  	fs_table_initialise(0, NULL, 0, NULL);
>  	fs = fs_table_lookup(argv[optind], FS_MOUNT_POINT);
> -	if (!fs) {
> +	if (!fs || (strcmp(argv[optind], fs->fs_dir) != 0)) {
>  		fprintf(stderr, _("%s: %s is not a mounted XFS filesystem\n"),
>  			progname, argv[optind]);
>  		return 1;
> 

Looks like the right start, but I see a few problems with this.

First, fs_dir contains a full path, so if we point at a relative path, this
will fail now (with a little extra error annotation):

# growfs/xfs_growfs -n mnt
xfs_growfs: mnt is not a mounted XFS filesystem (failed compare to /mnt/test/mnt)

but will work if given a full path:
# growfs/xfs_growfs -n `pwd`/mnt
meta-data=/dev/loop0             isize=512    agcount=4, agsize=65536 blks
...

The path stuff in libxcmd tries to boil everything down to a realpath() - see:
 
050a7f1 xfsprogs: handle symlinks etc in fs_table_initialise_mounts()
and
ed350fc libxcmd: make all comparisons using realpath'd paths

so if you want to compare paths to what's in the fstable, you'll need
to realpath() them first to get the the same canonical pathname 
that it has stored.

Also, if we have the same filesystem mounted
in multiple ways, say: via loop, via a secondary mount, and via a
bind mount (and maybe via a symlinked mount point?):

/mnt/test/fsfile on /mnt/test/mnt type xfs (rw,loop=/dev/loop0)
/dev/loop0 on /mnt/test/mnt2 type xfs (rw)
/mnt/test/mnt2 on /mnt/test/mnt3 type none (rw,bind)

it will fail on all but the first one it finds in the internal table:

# growfs/xfs_growfs -n `pwd`/mnt2
xfs_growfs: /mnt/test/mnt2 is not a mounted XFS filesystem (failed compare to /mnt/test/mnt)
# growfs/xfs_growfs -n `pwd`/mnt3
xfs_growfs: /mnt/test/mnt3 is not a mounted XFS filesystem (failed compare to /mnt/test/mnt)

because it the lookup stats the path, and picks the /first/ fstable entry
which has a matching device, ignoring the mountpoint found in the fstable entry.

So I think all that indicates an xfstest is needed to validate the change,
and a slightly different approach to finding a match.

Thanks,
-Eric