xfs
[Top] [All Lists]

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

To: <sekharan@xxxxxxxxxx>
Subject: Re: [PATCH] xfs: check the return value of xfs_trans_get_buf()
From: Alex Elder <aelder@xxxxxxx>
Date: Mon, 19 Sep 2011 11:36:53 -0500
Cc: XFS Mailing List <xfs@xxxxxxxxxxx>
In-reply-to: <1315411298.9298.5.camel@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
References: <1315411298.9298.5.camel@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
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
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:
                        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".

>                       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>