<div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Apr 1, 2016 at 4:05 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:0 0 0 .8ex;border-left:1px #ccc 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></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
</span>Just FYI - generally, the patch changelog goes below the "---"<br>
so it doesn't end up as part of the changelog in git.<br>
<br></blockquote><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">​Good idea, next time it should be there.​</div></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><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>
> -             finobt = 0;<br>
>       }<br>
<br>
</span>Is there any other case in mkfs where options are automatically disabled?<br>
I don't think so .. I'd just prefer a failure here, not a fix-up, even<br>
with the warning.  But I guess that's how it was before, so probably<br>
not something to change in this patch.  So never mind.  :)<br>
<br></blockquote><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">​Well, it was so, but as I'm trying to get rid of inconsistencies, I changed it to a failure if both crc=0 and finobt=1 are explicitly used. </div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
But, do we need the extra indentation?<br>
<br>
                if (sb_feat.finobt && sb_feat.finobtflag) {<br>
                        fprintf(stderr,<br>
<span class="">_("warning: finobt not supported without CRC support, disabled.\n"));<br>
                }<br>
</span>                sb_feat.finobt = 0;<br>
<br>
would suffice as before, no? Meh.  Not a big deal I guess....<br></blockquote><div> </div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">​Changed. Honestly, I don't like the strings starting at the beginning of the line, because it breaks the indentation flow, but the rest of the code uses this style, so I should stick to it.​</div> </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> @@ -2962,7 +3038,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),<br>
>               /*<br>
>                * Free INO btree root block<br>
>                */<br>
> -             if (!finobt) {<br>
> +             if (!sb_feat.finobt){<br>
</span>                                    ^ please fix whitespace :)<div class="HOEnZb"><div class="im trimless-h5 trimless-content"></div></div></blockquote><div><br></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">​Done.</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">Thank you for the review. I will wait a little longer if someone spots something more, before sending an updated patchset.​ :-)</div><br></div><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><br></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>