On Wed, Nov 09, 2022 at 12:28:09PM +0100, Carlos Maiolino wrote:
> On Mon, Sep 05, 2022 at 09:05:28AM +0200, Carlos Maiolino wrote:
> > On Fri, Sep 02, 2022 at 01:48:21PM -0700, Darrick J. Wong wrote:
> > > On Fri, Sep 02, 2022 at 03:43:39PM +0200, Carlos Maiolino wrote:
> > > > From: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
> > > >
> > > > Add proper exit error paths to avoid checking all pointers at the current path
> > > >
> > > > Fixes-coverity-id: 1512651
> > > >
> > > > Signed-off-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
> > > > ---
> > > > repair/rmap.c | 23 +++++++++++------------
> > > > 1 file changed, 11 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/repair/rmap.c b/repair/rmap.c
> > > > index a7c4b25b1..0253c0c36 100644
> > > > --- a/repair/rmap.c
> > > > +++ b/repair/rmap.c
> > > > @@ -1377,7 +1377,7 @@ check_refcounts(
> > > > if (error) {
> > > > do_warn(_("Could not read AGF %u to check refcount btree.\n"),
> > > > agno);
> > > > - goto err;
> > > > + goto err_agf;
> > >
> > > Shouldn't this ^^^^^^^ be err_pag, since we're erroring out and
> > > releasing the perag group reference?
> >
> > At first I named it err_pag, but pag is used here only to read the agf, and when
> > reading agf fail is why we end up reaching this error path, so I thought it
> > would be more specific to name it err_agf.
The kernel error-out label naming convention (AFAICT) is that the label
says what is being cleaned up. For example, xfs_create():
out_trans_cancel:
xfs_trans_cancel(tp);
out_release_inode:
/*
* Wait until after the current transaction is aborted to finish
* the
* setup of the inode and release the inode. This prevents
* recursive
* transactions and deadlocks from xfs_inactive.
*/
if (ip) {
xfs_finish_inode_setup(ip);
xfs_irele(ip);
}
out_release_dquots:
xfs_qm_dqrele(udqp);
xfs_qm_dqrele(gdqp);
xfs_qm_dqrele(pdqp);
if (unlock_dp_on_error)
xfs_iunlock(dp, XFS_ILOCK_EXCL);
return error;
}
> > > Also ... don't the "if (XXX) free(XXX)" bits take care of all this?
> > >
> >
> > Yeah, it does. But that's exactly what coverity is complaining about. We check
> > for a NULL pointer 'after' we dereference it earlier, to be more specific:
> >
> > ---
> > Type: Dereference before NULL check
> > Null-checking pag suggests that it may be null, but it has already been
> > dereferenced on all paths leading to the check
> > ---
> >
> > Both patches fix the same issue type.
Eh, I suppose it does get the coding style closer to how things are done
most other places, so it's a good idea.
Also, while you're at it, the btree cursor deletion function has long
accepted negative (and positive) errno as the second argument, so you
can turn it into:
libxfs_btree_del_cursor(bt_cur, error);
No need for XFS_BTREE_{NO,}ERROR.
> > > (I can't access Coverity any more, so I don't know what's in the
> > > report.)
> > >
> > > --D
> > >
> > > > }
>
> Hi Darrick. Do you have any other opinion at this? Or should I consider it a
> no-no and discard those patches?
Sorry, I guess I forgot to reply to you. :(
--D
> Cheers.
>
> --
> Carlos Maiolino
