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

[Jan Tulak]

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

> On 3/24/16 6:15 AM, jtulak at 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 at redhat.com / jan at tulak.me
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://oss.sgi.com/pipermail/xfs/attachments/20160406/f3eacacc/attachment.html>