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

[Jan Tulak <jtulak@xxxxxxxxxx>]

ââ

On Thu, Apr 7, 2016 at 2:12 AM, Eric SandeenÂ<sandeen@xxxxxxxxxxx>Âwrote:

On 3/24/16 6:15 AM,Âjtulak@xxxxxxxxxxÂwrote:
> @@ -1262,10 +1358,11 @@ main(
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âswitch (getsubopt(&p, (constpp)iopts, &value)) {
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âcase I_ALIGN:
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âif (!value || *value == '\0')
> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âvalue = "1";
> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âiaflag = atoi(value);
> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âif (iaflag < 0 || iaflag > 1)
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âreqval('i', iopts, I_ALIGN);
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âc = atoi(value);
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âif (c < 0 || c > 1)
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âillegal(value, "i align");
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âsb_feat.inode_align = c ? true : false;
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âbreak;
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âcase I_LOG:
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âif (!value || *value == '\0')


Hm, this seems wrong, as well - per the man page:

"If the value is omitted, 1 is assumed."

but this change with the reqval() removes that, doesn't it? Why?
(it's fixed later, but there is no reason to break it mid-series...)

â

Changed back to default 1. âAs for the origin of the change, most likely a copy&paste from some other place, where wasn't a default value, or it was there since I took over the patchset.Â

On Thu, Apr 7, 2016 at 3:43 AM, Eric Sandeen <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. :-)

Â

...

> @@ -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>
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. :-)â

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.

...

> @@ -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"...â

Cheers,

Jan

--

Jan Tulak

jtulak@xxxxxxxxxxÂ/ jan@xxxxxxxx