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

[Brian Foster <bfoster@redhat.com>]

On Fri, Jul 03, 2015 at 05:53:27AM -0400, Jan Tulak wrote:
> 
> 
> ----- Original Message -----
> > From: "Brian Foster" <bfoster@redhat.com>
> > > @@ -1912,17 +2013,17 @@ _("32 bit Project IDs always enabled on CRC enabled
> > > filesytems\n"));
> > >  		 * tried to use crc=0,finobt=1, then issue a warning before
> > >  		 * turning them off.
> > >  		 */
> > > -		if (finobt && finobtflag) {
> > > +		if (sb_feat.finobt && sb_feat.finobtflag) {
> > 
> > Since the code above drops finobtflag, I don't think we'll ever hit
> > this. Indeed, I can now create a crc=0,finobt=1 fs, which shouldn't
> > happen.
> > 
> > Brian
> > 
> 
> Finobtflag is dropped by a later patch in the set entirely. After all patches, the line is:
> 
> 	if (sb_feat.finobt && mopts.subopt_params[M_FINOBT].seen)
> 
> Which indeed works as it should:
> 
> mkfs.xfs -f -m crc=0,finobt=1 /dev/vdb2
> warning: finobt not supported without CRC support, disabled.
> 

Ok, fair enough. That said, I'm not a huge fan of letting broken patches
through just because things are fixed up in a subsequent patch. For one,
it makes review difficult and kind of removes incentive to review
individual patches rather than just the end result. In general, that's
problematic for things like future bisects or if you consider a
subsequent patch might be reverted down the line after all this context
is lost, re-exposing a previously known problem.

This particular instance is not a big deal. It requires the user to do
something wrong and we wouldn't mount the fs anyways. I'm just pointing
this out because IIRC there were a couple of instances of this "break in
one patch, fix in another" pattern in this series. In most cases, the
right thing to do is fix up the broken patch. ;)

Brian

> Cheers,
> Jan
> 
> -- 
> Jan Tulak
> jtulak@redhat.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs