xfs
[Top] [All Lists]

Re: [PATCH] mkfs.xfs: fix ftype-vs-crc option combination testing

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH] mkfs.xfs: fix ftype-vs-crc option combination testing
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Fri, 7 Aug 2015 07:37:42 -0400
Cc: xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <55C43FBA.1080408@xxxxxxxxxxx>
References: <55C43FBA.1080408@xxxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
On Thu, Aug 06, 2015 at 10:18:50PM -0700, Eric Sandeen wrote:
> mkfs.xfs got weird along the way; today it has different outcomes
> depending on the order of option specification:
> 
> $ mkfs/mkfs.xfs -n ftype=1 -m crc=0 -dfile,name=fsfile,size=16g
> cannot specify both crc and ftype
> $ mkfs/mkfs.xfs -m crc=0 -n ftype=1 -dfile,name=fsfile,size=16g
> <succeeds>
> 
> Somehow the tests got written as being constrained on what options
> are specified - and in what order! - vs actually testing for
> incompatible feature sets.
> 

IIRC, I think this is one of the core problems the big mkfs option
parsing rework that Jan is working on is supposed to fix.

> It's fine to specify both crc & ftype options, as long as it's an
> allowed combination, so just test for the incompatible combination
> (crc=1 and ftype=0) after all options have been processed.
> 
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---

Seems fine to me...

Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>

> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 69d61c7..9042796 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1477,11 +1477,6 @@ main(
>                                       if (c < 0 || c > 1)
>                                               illegal(value, "m crc");
>                                       crcs_enabled = c;
> -                                     if (nftype && crcs_enabled) {
> -                                             fprintf(stderr,
> -_("cannot specify both crc and ftype\n"));
> -                                             usage();
> -                                     }
>                                       break;
>                               case M_FINOBT:
>                                       if (!value || *value == '\0')
> @@ -1555,11 +1550,6 @@ _("cannot specify both crc and ftype\n"));
>                                       if (nftype)
>                                               respec('n', nopts, N_FTYPE);
>                                       dirftype = atoi(value);
> -                                     if (crcs_enabled) {
> -                                             fprintf(stderr,
> -_("cannot specify both crc and ftype\n"));
> -                                             usage();
> -                                     }
>                                       nftype = 1;
>                                       break;
>                               default:
> @@ -1714,6 +1704,10 @@ _("Minimum block size for CRC enabled filesystems is 
> %d bytes.\n"),
>                       XFS_MIN_CRC_BLOCKSIZE);
>               usage();
>       }
> +     if (crcs_enabled && !dirftype) {
> +             fprintf(stderr, _("cannot disable ftype with crcs enabled\n"));
> +             usage();
> +     }
>  
>       memset(&ft, 0, sizeof(ft));
>       get_topology(&xi, &ft, force_overwrite);
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

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