On Fri, Apr 07, 2017 at 01:51:47PM -0500, Eric Sandeen wrote:
> 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@xxxxxxxxxx>
> > ---
> > 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.
I submitted a v2. I'll also work on an xfstest.
Thanks-
Bill
>
> Thanks,
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
