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.
Ok, so for this patch, nothing but the mechanical change matching all the others,
>
> ...
>
> > @@ -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"...
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
_______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
