public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Check return value of xfs_buf_read()
@ 2011-08-03  2:18 Chandra Seetharaman
  2011-08-03  2:18 ` [PATCH 1/2] xfs: Check the return value of xfs_buf_read() for NULL Chandra Seetharaman
  2011-08-03  2:18 ` [PATCH 2/2] xfs: replace xfs_buf_geterror() with bp->b_error Chandra Seetharaman
  0 siblings, 2 replies; 5+ messages in thread
From: Chandra Seetharaman @ 2011-08-03  2:18 UTC (permalink / raw)
  To: xfs; +Cc: Chandra Seetharaman

Hello All,

While doing the previous clean up Alex noted that some of the places
where xfs_buf_read() returns, the return value is not checked for NULL.

This set check for the return value and returns failure if NULL and also
avoids using xfs_buf_geterror() when not needed.

Thanks & Regards,

chandra

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH 1/2] xfs: Check the return value of xfs_buf_read() for NULL
  2011-08-03  2:18 [PATCH 0/2] Check return value of xfs_buf_read() Chandra Seetharaman
@ 2011-08-03  2:18 ` Chandra Seetharaman
  2011-08-11 22:04   ` Alex Elder
  2011-08-03  2:18 ` [PATCH 2/2] xfs: replace xfs_buf_geterror() with bp->b_error Chandra Seetharaman
  1 sibling, 1 reply; 5+ messages in thread
From: Chandra Seetharaman @ 2011-08-03  2:18 UTC (permalink / raw)
  To: xfs; +Cc: Chandra Seetharaman

Check the return value of xfs_buf_read() for NULL and return ENOMEM if it is NULL.

Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
---
 fs/xfs/xfs_log_recover.c |    6 ++++++
 fs/xfs/xfs_rw.c          |    2 +-
 fs/xfs/xfs_vnodeops.c    |    2 ++
 3 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 93786e5..8478532 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2131,6 +2131,8 @@ xlog_recover_buffer_pass2(
 
 	bp = xfs_buf_read(mp->m_ddev_targp, buf_f->blf_blkno, buf_f->blf_len,
 			  buf_flags);
+	if (!bp)
+		return XFS_ERROR(ENOMEM);
 	error = xfs_buf_geterror(bp);
 	if (error) {
 		xfs_ioerror_alert("xlog_recover_do..(read#1)", mp,
@@ -2222,6 +2224,10 @@ xlog_recover_inode_pass2(
 
 	bp = xfs_buf_read(mp->m_ddev_targp, in_f->ilf_blkno, in_f->ilf_len,
 			  XBF_LOCK);
+	if (!bp) {
+		error = ENOMEM;
+		goto error;
+	}
 	error = xfs_buf_geterror(bp);
 	if (error) {
 		xfs_ioerror_alert("xlog_recover_do..(read#2)", mp,
diff --git a/fs/xfs/xfs_rw.c b/fs/xfs/xfs_rw.c
index c96a8a0..ecaa60e 100644
--- a/fs/xfs/xfs_rw.c
+++ b/fs/xfs/xfs_rw.c
@@ -136,7 +136,7 @@ xfs_read_buf(
 
 	bp = xfs_buf_read(target, blkno, len, flags);
 	if (!bp)
-		return XFS_ERROR(EIO);
+		return (flags & XBF_TRYLOCK) ?  0 : XFS_ERROR(EIO);
 	error = bp->b_error;
 	if (!error && !XFS_FORCED_SHUTDOWN(mp)) {
 		*bpp = bp;
diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index 3ee5f8a..64a5835 100644
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -83,6 +83,8 @@ xfs_readlink_bmap(
 
 		bp = xfs_buf_read(mp->m_ddev_targp, d, BTOBB(byte_cnt),
 				  XBF_LOCK | XBF_MAPPED | XBF_DONT_BLOCK);
+		if (!bp)
+			return XFS_ERROR(ENOMEM);
 		error = xfs_buf_geterror(bp);
 		if (error) {
 			xfs_ioerror_alert("xfs_readlink",
-- 
1.7.1

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH 2/2] xfs: replace xfs_buf_geterror() with bp->b_error
  2011-08-03  2:18 [PATCH 0/2] Check return value of xfs_buf_read() Chandra Seetharaman
  2011-08-03  2:18 ` [PATCH 1/2] xfs: Check the return value of xfs_buf_read() for NULL Chandra Seetharaman
@ 2011-08-03  2:18 ` Chandra Seetharaman
  2011-08-11 22:04   ` Alex Elder
  1 sibling, 1 reply; 5+ messages in thread
From: Chandra Seetharaman @ 2011-08-03  2:18 UTC (permalink / raw)
  To: xfs; +Cc: Chandra Seetharaman

Since we just checked bp for NULL, it is ok to replace xfs_buf_geterror()
with bp->b_error in these places.

Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
---
 fs/xfs/xfs_log_recover.c |    4 ++--
 fs/xfs/xfs_vnodeops.c    |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 8478532..a9df667 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2133,7 +2133,7 @@ xlog_recover_buffer_pass2(
 			  buf_flags);
 	if (!bp)
 		return XFS_ERROR(ENOMEM);
-	error = xfs_buf_geterror(bp);
+	error = bp->b_error;
 	if (error) {
 		xfs_ioerror_alert("xlog_recover_do..(read#1)", mp,
 				  bp, buf_f->blf_blkno);
@@ -2228,7 +2228,7 @@ xlog_recover_inode_pass2(
 		error = ENOMEM;
 		goto error;
 	}
-	error = xfs_buf_geterror(bp);
+	error = bp->b_error;
 	if (error) {
 		xfs_ioerror_alert("xlog_recover_do..(read#2)", mp,
 				  bp, in_f->ilf_blkno);
diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index 64a5835..d465c94 100644
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -85,7 +85,7 @@ xfs_readlink_bmap(
 				  XBF_LOCK | XBF_MAPPED | XBF_DONT_BLOCK);
 		if (!bp)
 			return XFS_ERROR(ENOMEM);
-		error = xfs_buf_geterror(bp);
+		error = bp->b_error;
 		if (error) {
 			xfs_ioerror_alert("xfs_readlink",
 				  ip->i_mount, bp, XFS_BUF_ADDR(bp));
-- 
1.7.1

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] xfs: Check the return value of xfs_buf_read() for NULL
  2011-08-03  2:18 ` [PATCH 1/2] xfs: Check the return value of xfs_buf_read() for NULL Chandra Seetharaman
@ 2011-08-11 22:04   ` Alex Elder
  0 siblings, 0 replies; 5+ messages in thread
From: Alex Elder @ 2011-08-11 22:04 UTC (permalink / raw)
  To: Chandra Seetharaman; +Cc: xfs

On Tue, 2011-08-02 at 19:18 -0700, Chandra Seetharaman wrote:
> Check the return value of xfs_buf_read() for NULL and return ENOMEM if it is NULL.

You might add that the reason for doing this is that in
these spots the null buffer pointer would be blindly
dereferenced otherwise.

One piece of this patch needs to go, so please post an
updated version.  But you can consider this reviewed by me.

Reviewed-by: Alex Elder <aelder@sgi.com>

> Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
> ---
>  fs/xfs/xfs_log_recover.c |    6 ++++++
>  fs/xfs/xfs_rw.c          |    2 +-
>  fs/xfs/xfs_vnodeops.c    |    2 ++
>  3 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 93786e5..8478532 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2131,6 +2131,8 @@ xlog_recover_buffer_pass2(
>  
>  	bp = xfs_buf_read(mp->m_ddev_targp, buf_f->blf_blkno, buf_f->blf_len,
>  			  buf_flags);
> +	if (!bp)
> +		return XFS_ERROR(ENOMEM);
>  	error = xfs_buf_geterror(bp);
>  	if (error) {
>  		xfs_ioerror_alert("xlog_recover_do..(read#1)", mp,
> @@ -2222,6 +2224,10 @@ xlog_recover_inode_pass2(
>  
>  	bp = xfs_buf_read(mp->m_ddev_targp, in_f->ilf_blkno, in_f->ilf_len,
>  			  XBF_LOCK);
> +	if (!bp) {
> +		error = ENOMEM;
> +		goto error;
> +	}
>  	error = xfs_buf_geterror(bp);
>  	if (error) {
>  		xfs_ioerror_alert("xlog_recover_do..(read#2)", mp,
> diff --git a/fs/xfs/xfs_rw.c b/fs/xfs/xfs_rw.c
> index c96a8a0..ecaa60e 100644
> --- a/fs/xfs/xfs_rw.c
> +++ b/fs/xfs/xfs_rw.c
> @@ -136,7 +136,7 @@ xfs_read_buf(
>  
>  	bp = xfs_buf_read(target, blkno, len, flags);
>  	if (!bp)
> -		return XFS_ERROR(EIO);
> +		return (flags & XBF_TRYLOCK) ?  0 : XFS_ERROR(EIO);

This change doesn't belong in this patch.

As far as I can tell, no call to xfs_read_buf()
ever passes XBF_TRYLOCK in flags anyway (only
XBF_LOCK, XBF_DONT_BLOCK, and XBF_MAPPED).

>  	error = bp->b_error;
>  	if (!error && !XFS_FORCED_SHUTDOWN(mp)) {
>  		*bpp = bp;
> diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
> index 3ee5f8a..64a5835 100644
> --- a/fs/xfs/xfs_vnodeops.c
> +++ b/fs/xfs/xfs_vnodeops.c
> @@ -83,6 +83,8 @@ xfs_readlink_bmap(
>  
>  		bp = xfs_buf_read(mp->m_ddev_targp, d, BTOBB(byte_cnt),
>  				  XBF_LOCK | XBF_MAPPED | XBF_DONT_BLOCK);
> +		if (!bp)
> +			return XFS_ERROR(ENOMEM);
>  		error = xfs_buf_geterror(bp);
>  		if (error) {
>  			xfs_ioerror_alert("xfs_readlink",



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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] xfs: replace xfs_buf_geterror() with bp->b_error
  2011-08-03  2:18 ` [PATCH 2/2] xfs: replace xfs_buf_geterror() with bp->b_error Chandra Seetharaman
@ 2011-08-11 22:04   ` Alex Elder
  0 siblings, 0 replies; 5+ messages in thread
From: Alex Elder @ 2011-08-11 22:04 UTC (permalink / raw)
  To: Chandra Seetharaman; +Cc: xfs

On Tue, 2011-08-02 at 19:18 -0700, Chandra Seetharaman wrote:
> Since we just checked bp for NULL, it is ok to replace xfs_buf_geterror()
> with bp->b_error in these places.
> 
> Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>

Looks good.

Reviewed-by: Alex Elder <aelder@sgi.com>


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

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-08-11 22:04 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-08-03  2:18 [PATCH 0/2] Check return value of xfs_buf_read() Chandra Seetharaman
2011-08-03  2:18 ` [PATCH 1/2] xfs: Check the return value of xfs_buf_read() for NULL Chandra Seetharaman
2011-08-11 22:04   ` Alex Elder
2011-08-03  2:18 ` [PATCH 2/2] xfs: replace xfs_buf_geterror() with bp->b_error Chandra Seetharaman
2011-08-11 22:04   ` Alex Elder

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox