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

[Eric Sandeen <sandeen@redhat.com>]

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@lst.de>
> 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@redhat.com>.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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@redhat.com>

> ---
>  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@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs