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

[Dan Carpenter <dan.carpenter@oracle.com>]

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.

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.

regards,
dan carpenter