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
>
>
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
