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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
