xfs
[Top] [All Lists]

Re: [PATCH 2/6] libxfs/linux.c: Replace use of ustat by stat

To: Felix Janda <felix.janda@xxxxxxxxx>
Subject: Re: [PATCH 2/6] libxfs/linux.c: Replace use of ustat by stat
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 8 Sep 2016 10:00:15 +1000
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20160112195945.GC568@nyan>
References: <cover.1452627740.git.felix.janda@xxxxxxxxx> <20160112195945.GC568@nyan>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Jan 12, 2016 at 08:59:45PM +0100, Felix Janda wrote:
> ustat has been used to check whether a device file is mounted.
> The function is deprecated and not supported by uclibc and musl.
> Now do the check using the *mntent functions.
> 
> Based on patch by Natanael Copa <ncopa@xxxxxxxxxxxxxxx>.
> 
> Signed-off-by: Felix Janda <felix.janda@xxxxxxxxx>

So, nobody has bothered to actually test this patch...

$ sudo mkfs.xfs -f -m rmapbt=1 -i sparse=1 -b size=1k /dev/sdc
specified blocksize 1024 is less than device physical sector size 4096
switching to logical sector size 512
Usage: mkfs.xfs
/* blocksize */         [-b log=n|size=num]
/* metadata */          [-m crc=0|1,finobt=0|1,uuid=xxx,rmapbt=0|1]
....
$ sudo xfs_db /dev/sdc

fatal error -- couldn't initialize XFS library
$

And so on. Basic sanity testing fails.

> @@ -63,14 +61,27 @@ platform_check_ismounted(char *name, char *block, struct 
> stat64 *s, int verbose)
>               s = &st;
>       }
>  
> -     if (ustat(s->st_rdev, ust) >= 0) {
> +     strcpy(mounts, (!access(PROC_MOUNTED, R_OK)) ? PROC_MOUNTED : MOUNTED);
> +     if ((f = setmntent(mounts, "r")) == NULL) {
> +             fprintf(stderr,
> +                 _("%s: %s possibly contains a mounted filesystem\n"),
> +                 progname, name);
> +             return 1;
> +     }
> +     while ((mnt = getmntent(f)) != NULL) {
> +             if (stat64(mnt->mnt_dir, &mst) < 0)
> +                     continue;
> +             if (mst.st_dev != s->st_rdev)
> +                     continue;
> +
>               if (verbose)
>                       fprintf(stderr,
>                               _("%s: %s contains a mounted filesystem\n"),
>                               progname, name);
> -             return 1;
> +             break;
>       }
> -     return 0;
> +     endmntent(f);
> +     return mnt == NULL;

Because that return statement has the wrong logic. It should be:

+       return mnt != NULL;

to return the correct values.

In future, please do more than a compile test on your patches before
sending them to the list for review, no matter how simple and
obviously correct you think they are!

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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