xfs
[Top] [All Lists]

Re: [PATCH] xfs_fsr: Get the last mount on a specific mount point

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs_fsr: Get the last mount on a specific mount point
From: Eric Sandeen <sandeen@xxxxxxxxxx>
Date: Fri, 17 Feb 2012 15:36:48 -0800
Cc: Carlos Maiolino <cmaiolino@xxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20120213174240.GA3474@xxxxxxxxxxxxx>
References: <1328640076-12645-1-git-send-email-cmaiolino@xxxxxxxxxx> <20120213174240.GA3474@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0.1) Gecko/20120208 Thunderbird/10.0.1
On 2/13/12 9:42 AM, Christoph Hellwig wrote:
> Thanks for taking care of this, but this just seems to make the already
> horribly ugly code even worse.
> 
> What do you think about the version below?
> 
> ---
> From: Christoph Hellwig <hch@xxxxxx>
> Subject: fsr: fix /proc/mounts parsing
> 
> Make sure we do not reject an XFS root mount just because /dev/root is also
> listed in /proc/mounts.  The root cause for this was the awkward getmntany
> function, which is replaced with a broader reach find_mountpoint function
> which replace getmntany and the surrounding code from the main routine in
> a structured way.  This changes the flow from finding a mounted filesystem
> matching the argument and checking that it's XFS to find a mounted XFS
> filesystem and thus fixes the bug.
> 
> Based on analysis and an earlier patch from
> Carlos Maiolino <cmaiolino@xxxxxxxxxx>.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Looks ok to me, thanks for doing it properly :)

I like Carlos' suggestion for the comment cleanup, and might suggest that
the /* device */ and /* mount point */ comments remain too; it's obvious
to us now I guess but I think the landmarks are nice for a fresh read.
No biggie.

Reviewed-by: Eric Sandeen <sandeen@xxxxxxxxxx>

> ---
>  fsr/xfs_fsr.c |  142 
> +++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 72 insertions(+), 70 deletions(-)
> 
> Index: xfsprogs-dev/fsr/xfs_fsr.c
> ===================================================================
> --- xfsprogs-dev.orig/fsr/xfs_fsr.c   2012-02-12 16:30:07.286766766 -0800
> +++ xfsprogs-dev/fsr/xfs_fsr.c        2012-02-12 16:42:39.293447376 -0800
> @@ -109,7 +109,6 @@ static void tmp_init(char *mnt);
>  static char * tmp_next(char *mnt);
>  static void tmp_close(char *mnt);
>  int xfs_getgeom(int , xfs_fsop_geom_v1_t * );
> -static int getmntany(FILE *, struct mntent *, struct mntent *, struct stat64 
> *);
>  
>  xfs_fsop_geom_v1_t fsgeom;   /* geometry of active mounted system */
>  
> @@ -178,18 +177,73 @@ aborter(int unused)
>       exit(1);
>  }
>  
> +/*
> + * Check if the argument is either the device name or mountpoint of an XFS
> + * filesystem.  Note that we do not care about bind mounted regular files
> + * here - the code that handles defragmentation of invidual files takes care
> + * of that.
> + */
> +static char *
> +find_mountpoint(char *mtab, char *argname, struct stat64 *sb)
> +{
> +     struct mntent *t;
> +     struct stat64 ms;
> +     FILE *mtabp;
> +     char *mntp = NULL;
> +
> +     mtabp = setmntent(mtab, "r");
> +     if (!mtabp) {
> +             fprintf(stderr, _("%s: cannot read %s\n"),
> +                     progname, mtab);
> +             exit(1);
> +     }
> +
> +     while ((t = getmntent(mtabp))) {
> +             if (S_ISDIR(sb->st_mode)) {
> +                     if (stat64(t->mnt_dir, &ms) < 0)
> +                             continue;
> +                     if (sb->st_ino != ms.st_ino)
> +                             continue;
> +                     if (sb->st_dev != ms.st_dev)
> +                             continue;
> +             } else {
> +                     if (stat64(t->mnt_fsname, &ms) < 0)
> +                             continue;
> +                     if (sb->st_rdev != ms.st_rdev)
> +                             continue;
> +             }
> +
> +             if (strcmp(t->mnt_type, MNTTYPE_XFS) != 0)
> +                     continue;
> +
> +             /*
> +              * If we found an entry based on the device name make sure we
> +              * stat the mountpoint that the mtab gave actually is accessible
> +              * before using it.
> +              */
> +             if (S_ISBLK(sb->st_mode)) {
> +                     struct stat64 sb2;
> +
> +                     if (stat64(t->mnt_dir, &sb2) < 0)
> +                             continue;
> +             }
> +
> +             mntp = t->mnt_dir;
> +             break;
> +     }
> +
> +     endmntent(mtabp);
> +     return mntp;
> +}
> +
>  int
>  main(int argc, char **argv)
>  {
> -     struct stat64 sb, sb2;
> +     struct stat64 sb;
>       char *argname;
> -     char *cp;
>       int c;
> -     struct mntent mntpref;
> -     register struct mntent *mntp;
> -     struct mntent ment;
> +     char *mntp;
>       char *mtab = NULL;
> -     register FILE *mtabp;
>  
>       setlinebuf(stdout);
>       progname = basename(argv[0]);
> @@ -281,49 +335,26 @@ main(int argc, char **argv)
>       if (optind < argc) {
>               for (; optind < argc; optind++) {
>                       argname = argv[optind];
> -                     mntp = NULL;
> +
>                       if (lstat64(argname, &sb) < 0) {
>                               fprintf(stderr,
>                                       _("%s: could not stat: %s: %s\n"),
>                                       progname, argname, strerror(errno));
>                               continue;
>                       }
> -                     if (S_ISLNK(sb.st_mode) && stat64(argname, &sb2) == 0 &&
> -                         (S_ISBLK(sb2.st_mode) || S_ISCHR(sb2.st_mode)))
> -                             sb = sb2;
> -                     if (S_ISBLK(sb.st_mode) || (S_ISDIR(sb.st_mode))) {
> -                             if ((mtabp = setmntent(mtab, "r")) == NULL) {
> -                                     fprintf(stderr,
> -                                             _("%s: cannot read %s\n"),
> -                                             progname, mtab);
> -                                     exit(1);
> -                             }
> -                             bzero(&mntpref, sizeof(mntpref));
> -                             if (S_ISDIR(sb.st_mode))
> -                                     mntpref.mnt_dir = argname;
> -                             else
> -                                     mntpref.mnt_fsname = argname;
>  
> -                             if (getmntany(mtabp, &ment, &mntpref, &sb) &&
> -                                 strcmp(ment.mnt_type, MNTTYPE_XFS) == 0) {
> -                                     mntp = &ment;
> -                                     if (S_ISBLK(sb.st_mode)) {
> -                                             cp = mntp->mnt_dir;
> -                                             if (cp == NULL ||
> -                                                 stat64(cp, &sb2) < 0) {
> -                                                     fprintf(stderr, _(
> -                                             "%s: could not stat: %s: %s\n"),
> -                                                     progname, argname,
> -                                                     strerror(errno));
> -                                                     continue;
> -                                             }
> -                                             sb = sb2;
> -                                             argname = cp;
> -                                     }
> -                             }
> +                     if (S_ISLNK(sb.st_mode)) {
> +                             struct stat64 sb2;
> +
> +                             if (stat64(argname, &sb2) == 0 &&
> +                                 (S_ISBLK(sb2.st_mode) ||
> +                                  S_ISCHR(sb2.st_mode)))
> +                             sb = sb2;
>                       }
> +
> +                     mntp = find_mountpoint(mtab, argname, &sb);
>                       if (mntp != NULL) {
> -                             fsrfs(mntp->mnt_dir, 0, 100);
> +                             fsrfs(mntp, 0, 100);
>                       } else if (S_ISCHR(sb.st_mode)) {
>                               fprintf(stderr, _(
>                                       "%s: char special not supported: %s\n"),
> @@ -1639,35 +1670,6 @@ fsrprintf(const char *fmt, ...)
>  }
>  
>  /*
> - * emulate getmntany
> - */
> -static int
> -getmntany(FILE *fp, struct mntent *mp, struct mntent *mpref, struct stat64 
> *s)
> -{
> -     struct mntent *t;
> -     struct stat64 ms;
> -
> -     while ((t = getmntent(fp))) {
> -             if (mpref->mnt_fsname) {        /* device */
> -                     if (stat64(t->mnt_fsname, &ms) < 0)
> -                             continue;
> -                     if (s->st_rdev != ms.st_rdev)
> -                             continue;
> -             }
> -             if (mpref->mnt_dir) {           /* mount point */
> -                     if (stat64(t->mnt_dir, &ms) < 0)
> -                             continue;
> -                     if (s->st_ino != ms.st_ino || s->st_dev != ms.st_dev)
> -                             continue;
> -             }
> -             *mp = *t;
> -             break;
> -     }
> -     return (t != NULL);
> -}
> -
> -
> -/*
>   * Initialize a directory for tmp file use.  This is used
>   * by the full filesystem defragmentation when we're walking
>   * the inodes and do not know the path for the individual
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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