Re: [bug report] xfs: remove dfops param from internal bmap extent helpers

[Brian Foster <bfoster@redhat.com>]

On Thu, Jul 19, 2018 at 02:01:31PM +0300, Dan Carpenter wrote:
> Thanks for looking at this.  I suspected it might be something like this
> so I would only send an email like this when the code was very recent
> so hopefully you would know the answers off the top of your head so it
> wasn't too much bother.
> 

No problem..

> On Thu, Jul 19, 2018 at 06:51:02AM -0400, Brian Foster wrote:
> > >   2455          /* update reverse mappings */
> > >   2456          error = xfs_rmap_convert_extent(mp, dfops, ip, whichfork, new);
> > >   2457          if (error)
> > >   2458                  goto done;
> > >   2459  
> > >   2460          /* convert to a btree if necessary */
> > >   2461          if (xfs_bmap_needs_btree(ip, whichfork)) {
> > >   2462                  int     tmp_logflags;   /* partial log flag return val */
> > >   2463  
> > >   2464                  ASSERT(cur == NULL);
> > >   2465                  error = xfs_bmap_extents_to_btree(tp, ip, &cur, 0,
> > >                                                           ^^
> > > Existing code dereferences "tp" without checking for NULL.
> > > 
> > 
> > ... and we only get here when xfs_bmap_needs_btree() is true, which
> > requires whichfork != XFS_COW_FORK (because I don't think such "convert
> > only" cow fork changes are ever logged).
> > 
> > So I don't think this patch changes behavior in any way and technically
> > this is a false positive. That said, I'm not opposed to tweaking the
> > function-local logic if it facilitates static checking (and clarity,
> > more importantly).
> > 
> > It sounds to me that adding a '... && tp' check to a few of these spots
> > may quiet the checker, I'm just not sure how to verify. Can this be run
> > locally or triggered somehow to verify a potential fix?
> 
> That would silence the warning but I think the code is probably more
> readable as-is.  There is some more logic that I have been needing to
> add to Smatch which might help here...  I just haven't got around to it
> yet.  I think we just leave it as-is and re-think in a few years if the
> warning is still around.
> 

Yeah, I wasn't totally convinced that actually addressed the clarity
part, that was just the easiest thing to test. ;) I'm fine with leaving
it as is if the checker can (or will) deal with it (and nobody else
complains). Thanks.

Brian

> regards,
> dan carpenter
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html