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:

quoted

Check the return value of xfs_trans_get_buf() and fail appropriately.

Signed-off-by: Chandra Seetharaman <redacted>

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

. . .

quoted

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

quoted

 		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.

quoted

 			if (pathlen < byte_cnt) {
 				byte_cnt = pathlen;
 			}


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs