<div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_extra"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"></div><br><div class="gmail_quote">On Thu, Apr 7, 2016 at 2:12 AM, Eric Sandeen <span dir="ltr"><<a href="mailto:sandeen@sandeen.net" target="_blank">sandeen@sandeen.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">On 3/24/16 6:15 AM, <a href="mailto:jtulak@redhat.com">jtulak@redhat.com</a> wrote:<br></span><span class="">> @@ -1262,10 +1358,11 @@ main(<br>> switch (getsubopt(&p, (constpp)iopts, &value)) {<br>> case I_ALIGN:<br>> if (!value || *value == '\0')<br>> - value = "1";<br>> - iaflag = atoi(value);<br>> - if (iaflag < 0 || iaflag > 1)<br>> + reqval('i', iopts, I_ALIGN);<br>> + c = atoi(value);<br>> + if (c < 0 || c > 1)<br>> illegal(value, "i align");<br>> + sb_feat.inode_align = c ? true : false;<br>> break;<br>> case I_LOG:<br>> if (!value || *value == '\0')<br><br><br></span>Hm, this seems wrong, as well - per the man page:<br><br>"If the value is omitted, 1 is assumed."<br><br>but this change with the reqval() removes that, doesn't it? Why?<br>(it's fixed later, but there is no reason to break it mid-series...)</blockquote></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">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. </div><br><div class="gmail_quote">On Thu, Apr 7, 2016 at 3:43 AM, Eric Sandeen <span dir="ltr"><<a href="mailto:sandeen@sandeen.net" target="_blank">sandeen@sandeen.net</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">
> @@ -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>
</span>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><span class=""><br></span></blockquote><div> </div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">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. :-)</div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">
</span>...<br>
<span class=""><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>
</span>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>
Author: Eric Sandeen <<a href="mailto:sandeen@sandeen.net">sandeen@sandeen.net</a>><br>
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>
<span class="">cannot specify both crc and ftype<br>
</span>Usage: mkfs.xfs<br>
...<br>
<span class=""><br></span></blockquote><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">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. :-)</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">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.</div></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">...<br>
<span class=""><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>
</span>like I mentioned, just this, I think (assuming we like the silent turning<br>
off, but that would be a different patch):<br></blockquote><div> </div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Merging the conditions is indeed cleaner.</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">And I will change it to failure, if the conflicting options are given explicitly. Just a small patch adding "usage();" and removing "warning"...</div></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Cheers,</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Jan</div></div><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>