xfs
[Top] [All Lists]

Re: [PATCH] xfs: check the return value of xfs_trans_get_buf()

To: aelder@xxxxxxx
Subject: Re: [PATCH] xfs: check the return value of xfs_trans_get_buf()
From: Chandra Seetharaman <sekharan@xxxxxxxxxx>
Date: Mon, 19 Sep 2011 13:34:34 -0500
Cc: XFS Mailing List <xfs@xxxxxxxxxxx>
In-reply-to: <1316450213.2941.20.camel@doink>
Organization: IBM
References: <1315411298.9298.5.camel@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <1316450213.2941.20.camel@doink>
Reply-to: sekharan@xxxxxxxxxx
Thanks for the review, Alex.

On Mon, 2011-09-19 at 11:36 -0500, Alex Elder wrote:
> On Wed, 2011-09-07 at 11:01 -0500, Chandra Seetharaman wrote:
> > Check the return value of xfs_trans_get_buf() and fail appropriately.
> > 
> > Signed-off-by: Chandra Seetharaman <sekharan@xxxxxxxxxx>
> 
> Sorry it took a while to get to this.
> 
> I started following back some of the paths that
> now (with your patch) return ENOMEM where they
> might not have before.  But I soon gave up because
> it explodes a bit in the number of possibilities.
> I did verify that the places that now return ENOMEM

Yes, I did check the callers to verify that they handle errors.

> have callers that check for an error return, so I'm
> going to just trust that's OK and that you have
> ensured there aren't any spots that do something
> unwanted in the event ENOMEM gets returned.
> 
> I did find something that may be a problem, so
> I'd like you to take another look and either
> explain why it's OK, or send an update to correct
> it.
> 
> Thanks.
> 
>                                       -Alex
> 
> . . .
> 
> > diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
> > index c2ff0fc..a4f3624 100644
> > --- a/fs/xfs/xfs_vnodeops.c
> > +++ b/fs/xfs/xfs_vnodeops.c
> > @@ -308,6 +308,8 @@ xfs_inactive_symlink_rmt(
> >             bp = xfs_trans_get_buf(tp, mp->m_ddev_targp,
> >                     XFS_FSB_TO_DADDR(mp, mval[i].br_startblock),
> >                     XFS_FSB_TO_BB(mp, mval[i].br_blockcount), 0);
> > +           if (!bp) 
> > +                   goto error1;
> 
> In this function, simply going to error1 will result
> in a 0 getting returned to the caller, because error
> had value 0 at this point.  I think you want something
> more like:

oops. overlook on my part. Sorry, about that. will fix it.

>                       if (!bp) {
>                               error = ENOMEM;
>                               goto error1;
>                       }
> 
> 
> >             xfs_trans_binval(tp, bp);
> >     }
> >     /*
> > @@ -1648,7 +1650,8 @@ xfs_symlink(
> >                     byte_cnt = XFS_FSB_TO_B(mp, mval[n].br_blockcount);
> >                     bp = xfs_trans_get_buf(tp, mp->m_ddev_targp, d,
> >                                            BTOBB(byte_cnt), 0);
> > -                   ASSERT(!xfs_buf_geterror(bp));
> > +                   if (!bp)
> > +                           goto error2;
> 
> Same thing here.  I think you want to set error to
> something before the "goto error2".

Same here.
> 
> >                     if (pathlen < byte_cnt) {
> >                             byte_cnt = pathlen;
> >                     }
> > 
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@xxxxxxxxxxx
> > http://oss.sgi.com/mailman/listinfo/xfs
> 
> 
> 


<Prev in Thread] Current Thread [Next in Thread>