<div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><span style="font-family:arial,sans-serif">On Thu, Apr 7, 2016 at 3:18 PM, Eric Sandeen </span><span dir="ltr" style="font-family:arial,sans-serif"><<a href="mailto:sandeen@sandeen.net" target="_blank">sandeen@sandeen.net</a>></span><span style="font-family:arial,sans-serif"> wrote:</span><br></div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 4/7/16 8:09 AM, Jan Tulak wrote:<br>
><br>
...<br>
<span class=""><br>
> On Thu, Apr 7, 2016 at 3:43 AM, Eric Sandeen <<a href="mailto:sandeen@sandeen.net">sandeen@sandeen.net</a> <mailto:<a href="mailto:sandeen@sandeen.net">sandeen@sandeen.net</a>>> wrote:<br>
><br>
> > @@ -981,11 +1077,21 @@ main(<br>
> > int worst_freelist;<br>
> > libxfs_init_t xi;<br>
> > struct fs_topology ft;<br>
> > - int lazy_sb_counters;<br>
> > - int crcs_enabled;<br>
> > - int finobt;<br>
> > - bool finobtflag;<br>
> > - int spinodes;<br>
> > + struct sb_feat_args sb_feat = {<br>
> > + .finobt = 1,<br>
> > + .finobtflag = false,<br>
><br>
><br>
> should we really have "finobtflag" in this structure?<br>
> This structure should only carry feature selections, not feature<br>
> specification flags I think. Why is this the only such flag<br>
> in the structure?<br>
><br>
> Pretty sure finobtflag should stay a variable for now<br>
> just like lvflag (which goes with log_version).<br>
><br>
><br>
> It might be right to move it out, but the flag is removed few<br>
> patches later entirely. Is it worth of the work? I would say nah, let<br>
> it die where it is. :-)<br>
<br>
</span>Given that it doesn't seem to be a bug, I guess that might be ok,<br>
but in general introducing incorrect things and fixing them later<br>
in the series is strongly discouraged...<br>
<span class=""><br>
><br>
><br>
> ...<br>
><br>
> > @@ -1517,7 +1617,14 @@ main(<br>
> > c = atoi(value);<br>
> > if (c < 0 || c > 1)<br>
> > illegal(value, "m crc");<br>
> > - crcs_enabled = c;<br>
> > + if (c && nftype) {<br>
> > + fprintf(stderr,<br>
> > +_("cannot specify both crc and ftype\n"));<br>
> > + usage();<br>
><br>
> hm, why is conflict checking added? It's not what the commit says<br>
> the patch does.<br>
><br>
> It also regresses the bug I fixed in<br>
><br>
> commit b990de8ba4e2df2bc76a140799d3ddb4a0eac4ce<br>
</span>> Author: Eric Sandeen <<a href="mailto:sandeen@sandeen.net">sandeen@sandeen.net</a> <mailto:<a href="mailto:sandeen@sandeen.net">sandeen@sandeen.net</a>>><br>
<span class="">> Date: Tue Aug 18 17:53:17 2015 +1000<br>
><br>
> mkfs.xfs: fix ftype-vs-crc option combination testing<br>
><br>
> with this patch, it is broken again:<br>
><br>
> # mkfs/mkfs.xfs -m crc=0 -n ftype=1 -dfile,name=fsfile,size=16g<br>
> <success><br>
> # mkfs/mkfs.xfs -n ftype=1 -m crc=0 -dfile,name=fsfile,size=16g<br>
> cannot specify both crc and ftype<br>
> Usage: mkfs.xfs<br>
> ...<br>
><br>
> Because the patch is much older than your fix, and at the time it<br>
> was created, it is possible that there wasn't any such check... I<br>
> would call it the risk of necromancy. :-)<br>
<br>
</span>Most likely a forward-port or merge error I think.<br>
<span class=""><br>
> Anyway, I already fixed this issue in this cycle, and added the the<br>
> ftype, crc order into a test checking for options sanity. Just I<br>
> didn't submitted the change yet.<br>
<br>
</span>Ok, so it is fixed in your new version of this patch?<br></blockquote><div><br></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">Yes.</div></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline"></div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="im trimless-h5 trimless-content"><br>
><br>
> ...<br>
><br>
> > @@ -1879,23 +1988,25 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));<br>
> > } else {<br>
> > /*<br>
> > * The kernel doesn't currently support crc=0,finobt=1<br>
> > - * filesystems. If crcs are not enabled and the user has<br>
> > - * explicitly turned them off then silently turn them off<br>
> > - * to avoid an unnecessary warning. If the user explicitly<br>
> > - * tried to use crc=0,finobt=1, then issue a warning before<br>
> > - * turning them off.<br>
> > + * filesystems. If crcs are not enabled and the user has not<br>
> > + * explicitly turned finobt on, then silently turn it off to<br>
> > + * avoid an unnecessary warning. If the user explicitly tried<br>
> > + * to use crc=0,finobt=1, then issue a warning before turning<br>
> > + * them off.<br>
> > */<br>
> > - if (finobt && finobtflag) {<br>
> > - fprintf(stderr,<br>
> > -_("warning: finobt not supported without CRC support, disabled.\n"));<br>
> > + if (sb_feat.finobt){<br>
> > + if (sb_feat.finobtflag) {<br>
> > + fprintf(stderr,<br>
> > + _("warning: finobt not supported without CRC support, disabled.\n"));<br>
> > + }<br>
> > + sb_feat.finobt = 0;<br>
><br>
> like I mentioned, just this, I think (assuming we like the silent turning<br>
> off, but that would be a different patch):<br>
><br>
><br>
> Merging the conditions is indeed cleaner.<br>
><br>
> And I will change it to failure, if the conflicting options are given<br>
> explicitly. Just a small patch adding "usage();" and removing<br>
> "warning"...<br>
<br>
</div></div>Ok, so for this patch, nothing but the mechanical change matching all the others,<br>
right? If there is any change in behavior to be done, that should be a different patch.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">Exactly.</div></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline"></div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="im trimless-h5 trimless-content"><br>
-Eric<br>
<br>
_______________________________________________<br>
xfs mailing list<br>
<a href="mailto:xfs@oss.sgi.com">xfs@oss.sgi.com</a><br>
<a href="http://oss.sgi.com/mailman/listinfo/xfs" rel="noreferrer" target="_blank">http://oss.sgi.com/mailman/listinfo/xfs</a><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Jan Tulak<br></div><a href="mailto:jtulak@redhat.com" target="_blank">jtulak@redhat.com</a> / <a href="mailto:jan@tulak.me" target="_blank">jan@tulak.me</a></div></div></div></div>
</div></div>