xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] mkfs.xfs fix detection of empty devices
From: Eric Sandeen <sandeen@xxxxxxxxxx>
Date: Thu, 04 Feb 2010 14:15:01 -0600
Cc: xfs mailing list <xfs@xxxxxxxxxxx>, Jim Meyering <meyering@xxxxxxxxxx>
In-reply-to: <20100204194000.GA7229@xxxxxxxxxxxxx>
References: <4B6AF6FC.6030101@xxxxxxxxxx> <20100204194000.GA7229@xxxxxxxxxxxxx>
User-agent: Thunderbird 2.0.0.21 (X11/20090320)
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@xxxxxx>
> 
> 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@xxxxxxxxxx>

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:

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