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

[Jan Tulak]

​​

On Thu, Apr 7, 2016 at 2:12 AM, Eric Sandeen <sandeen at sandeen.net> wrote:

> On 3/24/16 6:15 AM, jtulak at redhat.com 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 at 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. :-)



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

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 at redhat.com / jan at tulak.me
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://oss.sgi.com/pipermail/xfs/attachments/20160407/e9784d0b/attachment.html>