On Fri, Apr 1, 2016 at 4:05 AM, Eric Sandeen <sandeen@xxxxxxxxxxx> wrote:
On 3/24/16 6:15 AM, jtulak@xxxxxxxxxx wrote:
Just FYI - generally, the patch changelog goes below the "---"
so it doesn't end up as part of the changelog in git.
Good idea, next time it should be there.
> } 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;
> }
> - finobt = 0;
> }
Is there any other case in mkfs where options are automatically disabled?
I don't think so .. I'd just prefer a failure here, not a fix-up, even
with the warning. But I guess that's how it was before, so probably
not something to change in this patch. So never mind. :)
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.
But, do we need the extra indentation?
if (sb_feat.finobt && sb_feat.finobtflag) {
fprintf(stderr,
_("warning: finobt not supported without CRC support, disabled.\n"));
}
sb_feat.finobt = 0;
would suffice as before, no? Meh. Not a big deal I guess....
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.
> @@ -2962,7 +3038,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
> /*
> * Free INO btree root block
> */
> - if (!finobt) {
> + if (!sb_feat.finobt){
^ please fix whitespace :)
Done.
Thank you for the review. I will wait a little longer if someone spots something more, before sending an updated patchset. :-)
Cheers,
Jan
_______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
