Re: [PATCH 03/19] mkfs: Sanitise the superblock feature macros

[Jan Tulak <jtulak@xxxxxxxxxx>]

On Thu, Apr 7, 2016 at 3:18 PM, Eric Sandeen <sandeen@xxxxxxxxxxx> wrote:

On 4/7/16 8:09 AM, Jan Tulak wrote:
>
...

> On Thu, Apr 7, 2016 at 3:43 AM, Eric Sandeen <sandeen@xxxxxxxxxxx <mailto:sandeen@xxxxxxxxxxx>> wrote:
>
>Â Â Â> @@ -981,11 +1077,21 @@ main(
>  Â>   Âint          Âworst_freelist;
>  Â>   Âlibxfs_init_t     Âxi;
>  Â>   Âstruct fs_topology   ft;
>  Â> -  Âint          Âlazy_sb_counters;
>  Â> -  Âint          Âcrcs_enabled;
>  Â> -  Âint          Âfinobt;
>  Â> -  Âbool          finobtflag;
>  Â> -  Âint          Âspinodes;
>  Â> +  Âstruct sb_feat_args  Âsb_feat = {
>Â Â Â> +Â Â Â Â Â Â Â.finobt = 1,
>Â Â Â> +Â Â Â Â Â Â Â.finobtflag = false,
>
>
>Â Â Âshould we really have "finobtflag" in this structure?
>Â Â ÂThis structure should only carry feature selections, not feature
>  Âspecification flags I think. Why is this the only such flag
>Â Â Âin the structure?
>
>Â Â ÂPretty sure finobtflag should stay a variable for now
>Â Â Âjust like lvflag (which goes with log_version).
>
>
> âIt might be right to move it outâ, but the flag is removed few
> patches later entirely. Is it worth of the work? I would say nah, let
> it die where it is. :-)

Given that it doesn't seem to be a bug, I guess that might be ok,
but in general introducing incorrect things and fixing them later
in the series is strongly discouraged...

>
>
>Â Â Â...
>
>Â Â Â> @@ -1517,7 +1617,14 @@ main(
>Â Â Â>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âc = atoi(value);
>Â Â Â>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âif (c < 0 || c > 1)
>Â Â Â>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âillegal(value, "m crc");
>Â Â Â> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âcrcs_enabled = c;
>Â Â Â> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âif (c && nftype) {
>Â Â Â> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âfprintf(stderr,
>Â Â Â> +_("cannot specify both crc and ftype\n"));
>Â Â Â> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âusage();
>
>  Âhm, why is conflict checking added? It's not what the commit says
>Â Â Âthe patch does.
>
>Â Â ÂIt also regresses the bug I fixed in
>
>Â Â Âcommit b990de8ba4e2df2bc76a140799d3ddb4a0eac4ce
>Â Â ÂAuthor: Eric Sandeen <sandeen@xxxxxxxxxxx <mailto:sandeen@xxxxxxxxxxx>>
>Â Â ÂDate:Â ÂTue Aug 18 17:53:17 2015 +1000
>
>Â Â Â Â Âmkfs.xfs: fix ftype-vs-crc option combination testing
>
>Â Â Âwith this patch, it is broken again:
>
>Â Â Â# mkfs/mkfs.xfs -m crc=0 -n ftype=1 -dfile,name=fsfile,size=16g
>Â Â Â<success>
>Â Â Â # mkfs/mkfs.xfs -n ftype=1 -m crc=0 -dfile,name=fsfile,size=16g
>Â Â Âcannot specify both crc and ftype
>Â Â ÂUsage: mkfs.xfs
>Â Â Â...
>
> âBecause the patch is much older than your fix, and at the time it
> was created, it is possible that there wasn't any such check... I
> would call it the risk of necromancy. :-)â

Most likely a forward-port or merge error I think.

> Anyway, I already fixed this issue in this cycle, and added the the
> ftype, crc order into a test checking for options sanity. Just I
> didn't submitted the change yet.

Ok, so it is fixed in your new version of this patch?

âYes.

â

Â

>
>Â Â Â...
>
>Â Â Â> @@ -1879,23 +1988,25 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
>Â Â Â>Â Â Â Â} else {
>Â Â Â>Â Â Â Â Â Â Â Â/*
>Â Â Â>Â Â Â Â Â Â Â Â * The kernel doesn't currently support crc=0,finobt=1
>Â Â Â> -Â Â Â Â Â Â Â * filesystems. If crcs are not enabled and the user has
>Â Â Â> -Â Â Â Â Â Â Â * explicitly turned them off then silently turn them off
>Â Â Â> -Â Â Â Â Â Â Â * to avoid an unnecessary warning. If the user explicitly
>Â Â Â> -Â Â Â Â Â Â Â * tried to use crc=0,finobt=1, then issue a warning before
>Â Â Â> -Â Â Â Â Â Â Â * turning them off.
>Â Â Â> +Â Â Â Â Â Â Â * filesystems. If crcs are not enabled and the user has not
>Â Â Â> +Â Â Â Â Â Â Â * explicitly turned finobt on, then silently turn it off to
>Â Â Â> +Â Â Â Â Â Â Â * avoid an unnecessary warning. If the user explicitly tried
>Â Â Â> +Â Â Â Â Â Â Â * to use crc=0,finobt=1, then issue a warning before turning
>Â Â Â> +Â Â Â Â Â Â Â * them off.
>Â Â Â>Â Â Â Â Â Â Â Â */
>Â Â Â> -Â Â Â Â Â Â Âif (finobt && finobtflag) {
>Â Â Â> -Â Â Â Â Â Â Â Â Â Â Âfprintf(stderr,
>Â Â Â> -_("warning: finobt not supported without CRC support, disabled.\n"));
>Â Â Â> +Â Â Â Â Â Â Âif (sb_feat.finobt){
>Â Â Â> +Â Â Â Â Â Â Â Â Â Â Âif (sb_feat.finobtflag) {
>Â Â Â> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âfprintf(stderr,
>Â Â Â> +Â Â Â_("warning: finobt not supported without CRC support, disabled.\n"));
>Â Â Â> +Â Â Â Â Â Â Â Â Â Â Â}
>Â Â Â> +Â Â Â Â Â Â Â Â Â Â Âsb_feat.finobt = 0;
>
>Â Â Âlike I mentioned, just this, I think (assuming we like the silent turning
>Â Â Âoff, but that would be a different patch):
>
>
> âMerging the conditions is indeed cleaner.
>
> And I will change it to failure, if the conflicting options are given
> explicitly. Just a small patch adding "usage();" and removing
> "warning"...â

Ok, so for this patch, nothing but the mechanical change matching all the others,
right? If there is any change in behavior to be done, that should be a different patch.

âExactly.

â

Â

-Eric

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

--

Jan Tulak

jtulak@xxxxxxxxxxÂ/ jan@xxxxxxxx