Re: [PATCH 03/19] mkfs: Sanitise the superblock feature macros

[Jan Tulak <jtulak@xxxxxxxxxx>]

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â

--

Jan Tulak

jtulak@xxxxxxxxxxÂ/ jan@xxxxxxxx