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

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

[Chandra Seetharaman]

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

Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
---
diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c
index 8fad960..58c3add 100644
--- a/fs/xfs/xfs_attr_leaf.c
+++ b/fs/xfs/xfs_attr_leaf.c
@@ -2948,6 +2948,8 @@ xfs_attr_leaf_freextent(xfs_trans_t **trans, xfs_inode_t *dp,
 			bp = xfs_trans_get_buf(*trans,
 					dp->i_mount->m_ddev_targp,
 					dblkno, dblkcnt, XBF_LOCK);
+			if (!bp)
+				return ENOMEM;
 			xfs_trans_binval(*trans, bp);
 			/*
 			 * Roll to next transaction.
diff --git a/fs/xfs/xfs_btree.c b/fs/xfs/xfs_btree.c
index 2b9fd38..28cc019 100644
--- a/fs/xfs/xfs_btree.c
+++ b/fs/xfs/xfs_btree.c
@@ -970,7 +970,8 @@ xfs_btree_get_buf_block(
 	*bpp = xfs_trans_get_buf(cur->bc_tp, mp->m_ddev_targp, d,
 				 mp->m_bsize, flags);
 
-	ASSERT(!xfs_buf_geterror(*bpp));
+	if (!*bpp)
+		return ENOMEM;
 
 	*block = XFS_BUF_TO_BLOCK(*bpp);
 	return 0;
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 3e2ccae..0c5fe66 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -402,8 +402,11 @@ xfs_qm_dqalloc(
 			       dqp->q_blkno,
 			       mp->m_quotainfo->qi_dqchunklen,
 			       0);
-	if (!bp || (error = xfs_buf_geterror(bp)))
+
+	error = xfs_buf_geterror(bp);
+	if (error)
 		goto error1;
+
 	/*
 	 * Make a chunk of dquots out of this buffer and log
 	 * the entire thing.
diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
index 9f24ec2..207e0b0 100644
--- a/fs/xfs/xfs_ialloc.c
+++ b/fs/xfs/xfs_ialloc.c
@@ -150,7 +150,7 @@ xfs_check_agi_freecount(
 /*
  * Initialise a new set of inodes.
  */
-STATIC void
+STATIC int
 xfs_ialloc_inode_init(
 	struct xfs_mount	*mp,
 	struct xfs_trans	*tp,
@@ -202,8 +202,8 @@ xfs_ialloc_inode_init(
 		fbuf = xfs_trans_get_buf(tp, mp->m_ddev_targp, d,
 					 mp->m_bsize * blks_per_cluster,
 					 XBF_LOCK);
-		ASSERT(!xfs_buf_geterror(fbuf));
-
+		if (!fbuf)
+			return ENOMEM;
 		/*
 		 * Initialize all inodes in this buffer and then log them.
 		 *
@@ -225,6 +225,7 @@ xfs_ialloc_inode_init(
 		}
 		xfs_trans_inode_alloc_buf(tp, fbuf);
 	}
+	return 0;
 }
 
 /*
@@ -369,9 +370,11 @@ xfs_ialloc_ag_alloc(
 	 * rather than a linear progression to prevent the next generation
 	 * number from being easily guessable.
 	 */
-	xfs_ialloc_inode_init(args.mp, tp, agno, args.agbno, args.len,
-			      random32());
+	error = xfs_ialloc_inode_init(args.mp, tp, agno, args.agbno,
+			args.len, random32());
 
+	if (error)
+		return error;
 	/*
 	 * Convert the results.
 	 */
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 7f237ba..d689253 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1644,7 +1644,7 @@ xfs_iunlink_remove(
  * inodes that are in memory - they all must be marked stale and attached to
  * the cluster buffer.
  */
-STATIC void
+STATIC int
 xfs_ifree_cluster(
 	xfs_inode_t	*free_ip,
 	xfs_trans_t	*tp,
@@ -1690,6 +1690,8 @@ xfs_ifree_cluster(
 					mp->m_bsize * blks_per_cluster,
 					XBF_LOCK);
 
+		if (!bp)
+			return ENOMEM;
 		/*
 		 * Walk the inodes already attached to the buffer and mark them
 		 * stale. These will all have the flush locks held, so an
@@ -1799,6 +1801,7 @@ retry:
 	}
 
 	xfs_perag_put(pag);
+	return 0;
 }
 
 /*
@@ -1878,10 +1881,10 @@ xfs_ifree(
 	dip->di_mode = 0;
 
 	if (delete) {
-		xfs_ifree_cluster(ip, tp, first_ino);
+		error = xfs_ifree_cluster(ip, tp, first_ino);
 	}
 
-	return 0;
+	return error;
 }
 
 /*
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;
 		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;
 			if (pathlen < byte_cnt) {
 				byte_cnt = pathlen;
 			}


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

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

[Alex Elder]

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@us.ibm.com>

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



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

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

[Chandra Seetharaman]

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@us.ibm.com>
> 
> 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@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> 
> 
> 


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