xfs
[Top] [All Lists]

Re: [PATCH V2] xfsprogs: libxcmd/paths: make all comparisons using realp

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH V2] xfsprogs: libxcmd/paths: make all comparisons using realpath'd paths
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Mon, 14 Jul 2014 08:34:30 -0400
Cc: Eric Sandeen <sandeen@xxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <53C090A0.9050403@xxxxxxxxxxx>
References: <53C09034.3070203@xxxxxxxxxx> <53C090A0.9050403@xxxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
On Fri, Jul 11, 2014 at 08:34:24PM -0500, Eric Sandeen wrote:
> Both mountpoints and devices can be symlinks, so given a path
> to look for, and mountpoints/devices from the system, use
> realpath() on *everything* before making the comparison to see
> if our path is a match.
> 
> So, with symlinks for mount points as well as for devices:
> 
> # ls -l /dev/mapper/testvg-lvol0 
> lrwxrwxrwx. 1 root root 7 Jul 11 19:24 /dev/mapper/testvg-lvol0 -> ../dm-3
> # ls -l /mnt/scratch2
> lrwxrwxrwx. 1 root root 12 Jul 11 19:57 /mnt/scratch2 -> /mnt/scratch
> 
> this should all work, and does now:
> 
> # xfs_quota -xc "report -h" /mnt/scratch2
> User quota on /mnt/scratch (/dev/mapper/testvg-lvol0)
>                         Blocks              
> User ID      Used   Soft   Hard Warn/Grace   
> ---------- --------------------------------- 
> root            0      0      0  00 [------]
> 
> # xfs_quota -xc "report -h" /mnt/scratch
> User quota on /mnt/scratch (/dev/mapper/testvg-lvol0)
>                         Blocks              
> User ID      Used   Soft   Hard Warn/Grace   
> ---------- --------------------------------- 
> root            0      0      0  00 [------]
> 
> # xfs_quota -xc "report -h" /dev/dm-3 
> User quota on /mnt/scratch (/dev/mapper/testvg-lvol0)
>                         Blocks              
> User ID      Used   Soft   Hard Warn/Grace   
> ---------- --------------------------------- 
> root            0      0      0  00 [------]
> 
> # xfs_quota -xc "report -h" /dev/mapper/testvg-lvol0 
> User quota on /mnt/scratch (/dev/mapper/testvg-lvol0)
>                         Blocks              
> User ID      Used   Soft   Hard Warn/Grace   
> ---------- --------------------------------- 
> root            0      0      0  00 [------]
> 
> The commit:
> 
> 050a7f1 xfsprogs: handle symlinks etc in fs_table_initialise_mounts()
> 
> tried to fix this earlier, but only worked one way;
> it compared the argument path in both given and realpath
> form to the paths in getmntent, but did not compare to
> the realpaths of the getmntent devices.
> 
> If we reduce everything, everywhere, to a realpath(), we've
> got our best shot at finding the match.
> 
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
> 
> V2: remove the quota/report.c change which snuck in...
> 
> diff --git a/libxcmd/paths.c b/libxcmd/paths.c
> index 7b0e434..8da3968 100644
> --- a/libxcmd/paths.c
> +++ b/libxcmd/paths.c
> @@ -269,6 +269,9 @@ out_nomem:
>  /*
>   * If *path is NULL, initialize the fs table with all xfs mount points in 
> mtab
>   * If *path is specified, search for that path in mtab
> + *
> + * Everything - path, devices, and mountpoints - are reduced to realpath()
> + * for comparison, but fs_table is populated with what comes from getmntent.
>   */
>  static int
>  fs_table_initialise_mounts(
> @@ -278,7 +281,7 @@ fs_table_initialise_mounts(
>       FILE            *mtp;
>       char            *fslog, *fsrt;
>       int             error, found;
> -     char            *rpath = NULL;
> +     char            *rpath = NULL, *rmnt_fsname = NULL, *rmnt_dir = NULL;
>  
>       error = found = 0;
>       fslog = fsrt = NULL;
> @@ -300,11 +303,16 @@ fs_table_initialise_mounts(
>       while ((mnt = getmntent(mtp)) != NULL) {
>               if (strcmp(mnt->mnt_type, "xfs") != 0)
>                       continue;
> +             free(rmnt_dir);
> +             if ((rmnt_dir = realpath(mnt->mnt_dir, NULL)) == NULL)
> +                     continue;
> +             free(rmnt_fsname);
> +             if ((rmnt_fsname = realpath(mnt->mnt_fsname, NULL)) == NULL)
> +                     continue;
> +
>               if (path &&
> -                 ((strcmp(path, mnt->mnt_dir) != 0) &&
> -                  (strcmp(path, mnt->mnt_fsname) != 0) &&
> -                  (strcmp(rpath, mnt->mnt_dir) != 0) &&
> -                  (strcmp(rpath, mnt->mnt_fsname) != 0)))
> +                 ((strcmp(rpath, rmnt_dir) != 0) &&
> +                  (strcmp(rpath, rmnt_fsname) != 0)))
>                       continue;
>               if (fs_extract_mount_options(mnt, &fslog, &fsrt))
>                       continue;
> @@ -317,6 +325,8 @@ fs_table_initialise_mounts(
>       }
>       endmntent(mtp);
>       free(rpath);
> +     free(rmnt_dir);
> +     free(rmnt_fsname);
>  
>       if (path && !found)
>               error = ENXIO;
> @@ -330,6 +340,9 @@ fs_table_initialise_mounts(
>  /*
>   * If *path is NULL, initialize the fs table with all xfs mount points in 
> mtab
>   * If *path is specified, search for that path in mtab
> + *
> + * Everything - path, devices, and mountpoints - are reduced to realpath()
> + * for comparison, but fs_table is populated with what comes from getmntinfo.
>   */
>  static int
>  fs_table_initialise_mounts(
> @@ -337,7 +350,7 @@ fs_table_initialise_mounts(
>  {
>       struct statfs   *stats;
>       int             i, count, error, found;
> -     char            *rpath = NULL;
> +     char            *rpath = NULL, *rmntfromname= NULL, *rmntonname= NULL;

A couple missing spaces before '=' here.

The fundamental change looks good, but the memory allocation handling
seems a little ugly to me. A 'next:' label in the loop that frees the
path buffers is cleaner IMO. Another option could be to put a couple
PATH_MAX buffers on the stack or allocate them directly to also
eliminate the realpath() return value assignment..?

Brian

>  
>       error = found = 0;
>       if ((count = getmntinfo(&stats, 0)) < 0) {
> @@ -354,11 +367,16 @@ fs_table_initialise_mounts(
>       for (i = 0; i < count; i++) {
>               if (strcmp(stats[i].f_fstypename, "xfs") != 0)
>                       continue;
> +             free(rmntfromname);
> +             if ((rmntfromname = realpath(stats[i].f_mntfromname, NULL)) == 
> NULL)
> +                     continue;
> +             free(rmntonname);
> +             if ((rmntfromname = realpath(stats[i].f_mntonname,  NULL)) == 
> NULL)
> +                     continue;
> +
>               if (path &&
> -                 ((strcmp(path, stats[i].f_mntonname) != 0) &&
> -                  (strcmp(path, stats[i].f_mntfromname) != 0) &&
> -                  (strcmp(rpath, stats[i].f_mntonname) != 0) &&
> -                  (strcmp(rpath, stats[i].f_mntfromname) != 0)))
> +                 ((strcmp(rpath, rmntonname) != 0) &&
> +                  (strcmp(rpath, rmntfromname) != 0)))
>                       continue;
>               /* TODO: external log and realtime device? */
>               (void) fs_table_insert(stats[i].f_mntonname, 0,
> @@ -370,6 +388,8 @@ fs_table_initialise_mounts(
>               }
>       }
>       free(rpath);
> +     free(rmntfromname);
> +     free(rmntonname);
>       if (path && !found)
>               error = ENXIO;
>  
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

<Prev in Thread] Current Thread [Next in Thread>