[PATCH] mkfs: don't try to detect filesystems on regular files via blkid

[PATCH] mkfs: don't try to detect filesystems on regular files via blkid

[Eric Sandeen]

from RH bug 
https://bugzilla.redhat.com/show_bug.cgi?id=561870

# dd if=/dev/zero of=k bs=1MB count=2 seek=20; mkfs.xfs k 
# mkfs.xfs: probe of k failed, cannot detect existing filesystem.
# mkfs.xfs: Use the -f option to force overwrite.

blkid fails to do a probe of a regular file.

I wish blkid would cope with this, but for now it might
be better to just turn it off.

Reported-by: Jim Meyering <meyering@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 9baf116..de87647 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -300,10 +300,15 @@ check_overwrite(
 	int		fd;
 	long long	size;
 	int		bsz;
+	struct stat	statbuf;
 
 	if (!device || !*device)
 		return 0;
 
+	/* blkid can't get info from a regular file */
+	if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode))
+		return 0;
+
 	ret = -1; /* will reset on success of all setup calls */
 
 	fd = open(device, O_RDONLY);

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

Re: [PATCH] mkfs: don't try to detect filesystems on regular files via blkid

[Jim Meyering]

Eric Sandeen wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=561870
>
> # dd if=/dev/zero of=k bs=1MB count=2 seek=20; mkfs.xfs k
> # mkfs.xfs: probe of k failed, cannot detect existing filesystem.
> # mkfs.xfs: Use the -f option to force overwrite.
>
> blkid fails to do a probe of a regular file.
>
> I wish blkid would cope with this, but for now it might
> be better to just turn it off.
>
> Reported-by: Jim Meyering <meyering@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 9baf116..de87647 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -300,10 +300,15 @@ check_overwrite(
>  	int		fd;
>  	long long	size;
>  	int		bsz;
> +	struct stat	statbuf;
>
>  	if (!device || !*device)
>  		return 0;
>
> +	/* blkid can't get info from a regular file */
> +	if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode))
> +		return 0;
> +
>  	ret = -1; /* will reset on success of all setup calls */
>
>  	fd = open(device, O_RDONLY);

Hi Eric,

Did you consider calling fstat after opening,
rather than "stat" before?

        /* blkid can't get info from a regular file */
        if (!fstat(fd, &statbuf) && S_ISREG(statbuf.st_mode)) {
                close(fd);
                return 0;
        }

That's slightly more efficient, and not prone to confusion
in the unlikely event that "device" changes inode between
the stat and the open calls.

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

Re: [PATCH] mkfs: don't try to detect filesystems on regular files via blkid

[Eric Sandeen]

Jim Meyering wrote:
> Eric Sandeen wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=561870
>>
>> # dd if=/dev/zero of=k bs=1MB count=2 seek=20; mkfs.xfs k
>> # mkfs.xfs: probe of k failed, cannot detect existing filesystem.
>> # mkfs.xfs: Use the -f option to force overwrite.
>>
>> blkid fails to do a probe of a regular file.
>>
>> I wish blkid would cope with this, but for now it might
>> be better to just turn it off.
>>
>> Reported-by: Jim Meyering <meyering@redhat.com>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index 9baf116..de87647 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -300,10 +300,15 @@ check_overwrite(
>>  	int		fd;
>>  	long long	size;
>>  	int		bsz;
>> +	struct stat	statbuf;
>>
>>  	if (!device || !*device)
>>  		return 0;
>>
>> +	/* blkid can't get info from a regular file */
>> +	if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode))
>> +		return 0;
>> +
>>  	ret = -1; /* will reset on success of all setup calls */
>>
>>  	fd = open(device, O_RDONLY);
> 
> Hi Eric,
> 
> Did you consider calling fstat after opening,
> rather than "stat" before?
> 
>         /* blkid can't get info from a regular file */
>         if (!fstat(fd, &statbuf) && S_ISREG(statbuf.st_mode)) {
>                 close(fd);
>                 return 0;
>         }
> 
> That's slightly more efficient, and not prone to confusion
> in the unlikely event that "device" changes inode between
> the stat and the open calls.

Ok, yes that's probably better, will resend.  thanks.

-Eric

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

[PATCH V2] mkfs: don't try to detect filesystems on regular files via blkid

[Eric Sandeen]

from RH bug 
https://bugzilla.redhat.com/show_bug.cgi?id=561870

# dd if=/dev/zero of=k bs=1MB count=2 seek=20; mkfs.xfs k 
# mkfs.xfs: probe of k failed, cannot detect existing filesystem.
# mkfs.xfs: Use the -f option to force overwrite.

blkid fails to do a probe of a regular file.

I wish blkid would cope with this, but for now it might
be better to just turn it off.

I kept the size==0 check just in case we stumble on a 0-sized
device, blkid doesn't like that either...

Reported-by: Jim Meyering <meyering@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: use fstat after the open

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 9baf116..219c81e 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -300,6 +300,7 @@ check_overwrite(
 	int		fd;
 	long long	size;
 	int		bsz;
+	struct stat	statbuf;
 
 	if (!device || !*device)
 		return 0;
@@ -309,6 +310,14 @@ check_overwrite(
 	fd = open(device, O_RDONLY);
 	if (fd < 0)
 		goto out;
+
+	/* blkid can't get info from a regular file */
+	if (!fstat(fd, &statbuf) && S_ISREG(statbuf.st_mode)) {
+		close(fd);
+		ret = 0;
+		goto out;
+	}
+
 	platform_findsizes(device, fd, &size, &bsz);
 	close(fd);
 


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

[PATCH] mkfs.xfs fix detection of empty devices

[Christoph Hellwig]

We currently fail to detect that a device does indeed not contain any
signature and we are indeed fine to proceed with it due to mishandling
the return value of blkid_do_fullprobe.  Fix that up and add some
better diagnostics of the blkid detection.  Also remove the size == 0
check in check_overwrite as blkid handles that just fine.

from RH bugzilla https://bugzilla.redhat.com/show_bug.cgi?id=561870

# dd if=/dev/zero of=k bs=1MB count=2 seek=20; mkfs.xfs k 
# mkfs.xfs: probe of k failed, cannot detect existing filesystem.
# mkfs.xfs: Use the -f option to force overwrite

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfsprogs-dev/mkfs/xfs_mkfs.c
===================================================================
--- xfsprogs-dev.orig/mkfs/xfs_mkfs.c	2010-02-04 19:19:36.000000000 +0000
+++ xfsprogs-dev/mkfs/xfs_mkfs.c	2010-02-04 19:33:36.000000000 +0000
@@ -297,48 +297,47 @@ check_overwrite(
 	const char	*type;
 	blkid_probe	pr = NULL;
 	int		ret;
-	int		fd;
-	long long	size;
-	int		bsz;
 
 	if (!device || !*device)
 		return 0;
 
 	ret = -1; /* will reset on success of all setup calls */
 
-	fd = open(device, O_RDONLY);
-	if (fd < 0)
-		goto out;
-	platform_findsizes(device, fd, &size, &bsz);
-	close(fd);
-
-	/* nothing to overwrite on a 0-length device */
-	if (size == 0) {
-		ret = 0;
-		goto out;
-	}
-
 	pr = blkid_new_probe_from_filename(device);
 	if (!pr)
 		goto out;
 
-	if (blkid_probe_enable_partitions(pr, 1))
+	ret = blkid_probe_enable_partitions(pr, 1);
+	if (ret < 0)
+		goto out;
+
+	ret = blkid_do_fullprobe(pr);
+	if (ret < 0)
 		goto out;
 
-	if (blkid_do_fullprobe(pr))
+	/*
+	 * Blkid returns 1 for nothing found and 0 when it finds a signature,
+	 * but we want the exact opposite, so reverse the return value here.
+	 *
+	 * In addition print some useful diagnostics about what actually is
+	 * on the device.
+	 */
+	ret = !ret;
+	if (!ret)
 		goto out;
 
-	ret = 0;
 	if (!blkid_probe_lookup_value(pr, "TYPE", &type, NULL)) {
 		fprintf(stderr,
 			_("%s: %s appears to contain an existing "
 			"filesystem (%s).\n"), progname, device, type);
-		ret = 1;
 	} else if (!blkid_probe_lookup_value(pr, "PTTYPE", &type, NULL)) {
 		fprintf(stderr,
 			_("%s: %s appears to contain a partition "
 			"table (%s).\n"), progname, device, type);
-		ret = 1;
+	} else {
+		fprintf(stderr,
+			_("%s: %s appears to contain something weird "
+			"according to blkid\n"), progname, device);
 	}
 
 out:

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

Re: [PATCH V2] mkfs: don't try to detect filesystems on regular files via blkid

[Eric Sandeen]

Eric Sandeen wrote:
> from RH bug 
> https://bugzilla.redhat.com/show_bug.cgi?id=561870
> 
> # dd if=/dev/zero of=k bs=1MB count=2 seek=20; mkfs.xfs k 
> # mkfs.xfs: probe of k failed, cannot detect existing filesystem.
> # mkfs.xfs: Use the -f option to force overwrite.
> 
> blkid fails to do a probe of a regular file.
> 
> I wish blkid would cope with this, but for now it might
> be better to just turn it off.
> 
> I kept the size==0 check just in case we stumble on a 0-sized
> device, blkid doesn't like that either...
> 
> Reported-by: Jim Meyering <meyering@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

OK NAK on that, hch has a better patch, we were driving blkid wrong.

-Eric

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

Re: [PATCH] mkfs.xfs fix detection of empty devices

[Eric Sandeen]

Christoph Hellwig wrote:
> We currently fail to detect that a device does indeed not contain any
> signature and we are indeed fine to proceed with it due to mishandling
> the return value of blkid_do_fullprobe.  Fix that up and add some
> better diagnostics of the blkid detection.  Also remove the size == 0
> check in check_overwrite as blkid handles that just fine.

Much better, thanks, minor comment below

> from RH bugzilla https://bugzilla.redhat.com/show_bug.cgi?id=561870
> 
> # dd if=/dev/zero of=k bs=1MB count=2 seek=20; mkfs.xfs k 
> # mkfs.xfs: probe of k failed, cannot detect existing filesystem.
> # mkfs.xfs: Use the -f option to force overwrite
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfsprogs-dev/mkfs/xfs_mkfs.c
> ===================================================================
> --- xfsprogs-dev.orig/mkfs/xfs_mkfs.c	2010-02-04 19:19:36.000000000 +0000
> +++ xfsprogs-dev/mkfs/xfs_mkfs.c	2010-02-04 19:33:36.000000000 +0000
> @@ -297,48 +297,47 @@ check_overwrite(
>  	const char	*type;
>  	blkid_probe	pr = NULL;
>  	int		ret;
> -	int		fd;
> -	long long	size;
> -	int		bsz;
>  
>  	if (!device || !*device)
>  		return 0;
>  
>  	ret = -1; /* will reset on success of all setup calls */
>  
> -	fd = open(device, O_RDONLY);
> -	if (fd < 0)
> -		goto out;
> -	platform_findsizes(device, fd, &size, &bsz);
> -	close(fd);
> -
> -	/* nothing to overwrite on a 0-length device */
> -	if (size == 0) {
> -		ret = 0;
> -		goto out;
> -	}
> -
>  	pr = blkid_new_probe_from_filename(device);
>  	if (!pr)
>  		goto out;
>  
> -	if (blkid_probe_enable_partitions(pr, 1))
> +	ret = blkid_probe_enable_partitions(pr, 1);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = blkid_do_fullprobe(pr);
> +	if (ret < 0)
>  		goto out;
>  
> -	if (blkid_do_fullprobe(pr))
> +	/*
> +	 * Blkid returns 1 for nothing found and 0 when it finds a signature,
> +	 * but we want the exact opposite, so reverse the return value here.
> +	 *
> +	 * In addition print some useful diagnostics about what actually is
> +	 * on the device.
> +	 */
> +	ret = !ret;
> +	if (!ret)
>  		goto out;

that makes my brain hurt a little.  Maybe:

if (ret == 1) {	/* blkid found nothing */
	ret = 0;	/* we return 0 for nothing found */
	goto out;
} else		/* blkid found something */
	ret = 1;	/* we return 1 for something found */

it's wordy but at least not confusing.

I'm ok either way, I guess, so:

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

unless you change anything enough that you want another review.  :)

-Eric

> -	ret = 0;
>  	if (!blkid_probe_lookup_value(pr, "TYPE", &type, NULL)) {
>  		fprintf(stderr,
>  			_("%s: %s appears to contain an existing "
>  			"filesystem (%s).\n"), progname, device, type);
> -		ret = 1;
>  	} else if (!blkid_probe_lookup_value(pr, "PTTYPE", &type, NULL)) {
>  		fprintf(stderr,
>  			_("%s: %s appears to contain a partition "
>  			"table (%s).\n"), progname, device, type);
> -		ret = 1;
> +	} else {
> +		fprintf(stderr,
> +			_("%s: %s appears to contain something weird "
> +			"according to blkid\n"), progname, device);
>  	}
>  
>  out:

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

Re: [PATCH v2] mkfs.xfs fix detection of empty devices

[Christoph Hellwig]

We currently fail to detect that a device does indeed not contain any
signature and we are indeed fine to proceed with it due to mishandling
the return value of blkid_do_fullprobe.  Fix that up and add some
better diagnostics of the blkid detection.

from RH bugzilla https://bugzilla.redhat.com/show_bug.cgi?id=561870

# dd if=/dev/zero of=k bs=1MB count=2 seek=20; mkfs.xfs k 
# mkfs.xfs: probe of k failed, cannot detect existing filesystem.
# mkfs.xfs: Use the -f option to force overwrite

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfsprogs-dev/mkfs/xfs_mkfs.c
===================================================================
--- xfsprogs-dev.orig/mkfs/xfs_mkfs.c	2010-02-04 20:30:26.000000000 +0000
+++ xfsprogs-dev/mkfs/xfs_mkfs.c	2010-02-04 20:43:41.000000000 +0000
@@ -322,24 +322,40 @@ check_overwrite(
 	if (!pr)
 		goto out;
 
-	if (blkid_probe_enable_partitions(pr, 1))
+	ret = blkid_probe_enable_partitions(pr, 1);
+	if (ret < 0)
 		goto out;
 
-	if (blkid_do_fullprobe(pr))
+	ret = blkid_do_fullprobe(pr);
+	if (ret < 0)
 		goto out;
 
-	ret = 0;
+	/*
+	 * Blkid returns 1 for nothing found and 0 when it finds a signature,
+	 * but we want the exact opposite, so reverse the return value here.
+	 *
+	 * In addition print some useful diagnostics about what actually is
+	 * on the device.
+	 */
+	if (ret) {
+		ret = 0;
+		goto out;
+	}
+
 	if (!blkid_probe_lookup_value(pr, "TYPE", &type, NULL)) {
 		fprintf(stderr,
 			_("%s: %s appears to contain an existing "
 			"filesystem (%s).\n"), progname, device, type);
-		ret = 1;
 	} else if (!blkid_probe_lookup_value(pr, "PTTYPE", &type, NULL)) {
 		fprintf(stderr,
 			_("%s: %s appears to contain a partition "
 			"table (%s).\n"), progname, device, type);
-		ret = 1;
+	} else {
+		fprintf(stderr,
+			_("%s: %s appears to contain something weird "
+			"according to blkid\n"), progname, device);
 	}
+	ret = 1;
 
 out:
 	if (pr)

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

Re: [PATCH v2] mkfs.xfs fix detection of empty devices

[Eric Sandeen]

Christoph Hellwig wrote:
> We currently fail to detect that a device does indeed not contain any
> signature and we are indeed fine to proceed with it due to mishandling
> the return value of blkid_do_fullprobe.  Fix that up and add some
> better diagnostics of the blkid detection.
> 
> from RH bugzilla https://bugzilla.redhat.com/show_bug.cgi?id=561870
> 
> # dd if=/dev/zero of=k bs=1MB count=2 seek=20; mkfs.xfs k 
> # mkfs.xfs: probe of k failed, cannot detect existing filesystem.
> # mkfs.xfs: Use the -f option to force overwrite
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

we discussed keeping the 0-size check until libblkid copes better;
also the absense of ret = !ret looks more readable, thanks.

Reviewed-by: Eric Sandeen <sandeen@sandeen.net>

> Index: xfsprogs-dev/mkfs/xfs_mkfs.c
> ===================================================================
> --- xfsprogs-dev.orig/mkfs/xfs_mkfs.c	2010-02-04 20:30:26.000000000 +0000
> +++ xfsprogs-dev/mkfs/xfs_mkfs.c	2010-02-04 20:43:41.000000000 +0000
> @@ -322,24 +322,40 @@ check_overwrite(
>  	if (!pr)
>  		goto out;
>  
> -	if (blkid_probe_enable_partitions(pr, 1))
> +	ret = blkid_probe_enable_partitions(pr, 1);
> +	if (ret < 0)
>  		goto out;
>  
> -	if (blkid_do_fullprobe(pr))
> +	ret = blkid_do_fullprobe(pr);
> +	if (ret < 0)
>  		goto out;
>  
> -	ret = 0;
> +	/*
> +	 * Blkid returns 1 for nothing found and 0 when it finds a signature,
> +	 * but we want the exact opposite, so reverse the return value here.
> +	 *
> +	 * In addition print some useful diagnostics about what actually is
> +	 * on the device.
> +	 */
> +	if (ret) {
> +		ret = 0;
> +		goto out;
> +	}
> +
>  	if (!blkid_probe_lookup_value(pr, "TYPE", &type, NULL)) {
>  		fprintf(stderr,
>  			_("%s: %s appears to contain an existing "
>  			"filesystem (%s).\n"), progname, device, type);
> -		ret = 1;
>  	} else if (!blkid_probe_lookup_value(pr, "PTTYPE", &type, NULL)) {
>  		fprintf(stderr,
>  			_("%s: %s appears to contain a partition "
>  			"table (%s).\n"), progname, device, type);
> -		ret = 1;
> +	} else {
> +		fprintf(stderr,
> +			_("%s: %s appears to contain something weird "
> +			"according to blkid\n"), progname, device);
>  	}
> +	ret = 1;
>  
>  out:
>  	if (pr)
> 
> _______________________________________________
> 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