* Re: [PATCH] xfs_fsr: Get the last mount on a specific mount point
2012-02-13 17:42 ` Christoph Hellwig
@ 2012-02-15 12:31 ` Carlos Maiolino
2012-02-17 17:26 ` Christoph Hellwig
2012-02-17 23:36 ` Eric Sandeen
1 sibling, 1 reply; 7+ messages in thread
From: Carlos Maiolino @ 2012-02-15 12:31 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
Hi,
> ---
> 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.
> + */
I would change a little this sentence, looks a little confusing at least for me.
How about:
/* If we found an entry based on the device name, make sure we
* stat the mount point given by mtab and ensure it's 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;
Particularly, I don't like variable declarations between statements, but I'm not
too experienced to say what's good or not :)
> +
> + 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
Otherwise looks good.
--
--Carlos
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] xfs_fsr: Get the last mount on a specific mount point
2012-02-13 17:42 ` Christoph Hellwig
2012-02-15 12:31 ` Carlos Maiolino
@ 2012-02-17 23:36 ` Eric Sandeen
2012-04-24 20:14 ` Carlos Maiolino
1 sibling, 1 reply; 7+ messages in thread
From: Eric Sandeen @ 2012-02-17 23:36 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Carlos Maiolino, xfs
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
^ permalink raw reply [flat|nested] 7+ messages in thread