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 _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
