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