Re: [PATCH] xfs_db: fix inverted logic in error path

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Apr 12, 2023 at 09:59:03AM +1000, Dave Chinner wrote:
> On Tue, Apr 11, 2023 at 10:46:44AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > smatch complains proceeding into the if body if leaf is a null pointer.
> > This is backwards, so correct that.
> > 
> > check.c:3455 process_leaf_node_dir_v2_int() warn: variable dereferenced before check 'leaf'
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  db/check.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/db/check.c b/db/check.c
> > index 964756d0..d5f3d225 100644
> > --- a/db/check.c
> > +++ b/db/check.c
> > @@ -3452,7 +3452,7 @@ process_leaf_node_dir_v2_int(
> >  				 id->ino, dabno, stale,
> >  				 be16_to_cpu(leaf3->hdr.stale));
> >  		error++;
> > -	} else if (!leaf && stale != be16_to_cpu(leaf->hdr.stale)) {
> > +	} else if (leaf && stale != be16_to_cpu(leaf->hdr.stale)) {
> >  		if (!sflag || v)
> >  			dbprintf(_("dir %lld block %d stale mismatch "
> >  				 "%d/%d\n"),
> 
> Looks good. I'm surprised the compiler didn't warn about this
> obviously broken code...
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

NAK, this logic is actually trying to switch between the V4 and V5
directory format logic, but it's using the subtlety that @leaf3 is a
null pointer for a V4 fs and non-null for a V5 fs.   Hence this check
really should be:

	} else if (!leaf3 && stale != be16_to_cpu(leaf->hdr.stale)) {

Even though that looks *totally* wrong.  But xfs_check is deprecated, so
fmeh.

So I'll go fix that and rerun through testing.  I had thought this was
fine, but xfs/291 barfed up errors overnight.

--D

> -- 
> Dave Chinner
> david@fromorbit.com