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

[Eric Sandeen <sandeen@sandeen.net>]

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

> On Thu, Apr 7, 2016 at 3:43 AM, Eric Sandeen <sandeen@sandeen.net <mailto:sandeen@sandeen.net>> 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@sandeen.net <mailto:sandeen@sandeen.net>>
>     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?

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

-Eric

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