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

Jan Tulak
2016-04-06T09:12:45Z

[-- Attachment #1.1: Type: text/plain, Size: 3278 bytes --]

On Fri, Apr 1, 2016 at 4:05 AM, Eric Sandeen <sandeen@sandeen.net> wrote:

> On 3/24/16 6:15 AM, jtulak@redhat.com 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@redhat.com / jan@tulak.me

[-- Attachment #1.2: Type: text/html, Size: 5962 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]