[Cluster-devel] [gfs2:master 3/5] fs/gfs2/rgrp.c:1946 gfs2_inplace_reserve() warn: variable dereferenced before check 'ap' (see line 1938)

[Cluster-devel] [gfs2:master 3/5] fs/gfs2/rgrp.c:1946 gfs2_inplace_reserve() warn: variable dereferenced before check 'ap' (see line 1938)

[Steven Whitehouse]

Hi,

On Tue, 2013-12-03 at 14:01 +0300, Dan Carpenter wrote:
[snip]
> > > 90306c41 Benjamin Marzinski 2012-05-29  1939  				}
> > > 90306c41 Benjamin Marzinski 2012-05-29  1940  			}
> > > 292c8c14 Abhijith Das       2007-11-29  1941  		}
> > > 666d1d8a Bob Peterson       2012-06-13  1942  
> > > c743ffd0 Steven Whitehouse  2012-08-25  1943  		/* Skip unuseable resource groups */
> > > 2d7e8aac Bob Peterson       2013-11-25  1944  		if ((rs->rs_rbm.rgd->rd_flags & (GFS2_RGF_NOALLOC |
> > > 2d7e8aac Bob Peterson       2013-11-25  1945  						 GFS2_RDF_ERROR)) ||
> > > 2d7e8aac Bob Peterson       2013-11-25 @1946  		    (ap && (ap->target > rs->rs_rbm.rgd->rd_extfail_pt)))
> >  ... and this looks ok to me.
> > 
> > Can you give some more detail as to whats wrong here?
> 
> The check that was introduced could be removed because this function is
> always called with a valid "ap" and we dereferenced it already.
> 
> regards,
> dan carpenter
> 

Ah, now I see what it is referring to. How does the following look as a
fix?



From c599ca2dc6f554a3042e83026b3ecf3441988915 Mon Sep 17 00:00:00 2001
From: Steven Whitehouse <swhiteho@redhat.com>
Date: Wed, 4 Dec 2013 11:14:05 +0000
Subject: [PATCH] GFS2: Remove test which is always true

Since gfs2_inplace_reserve() is always called with a valid
alloc parms structure, there is no need to test for this
within the function itself - and in any case, after we've
all ready dereferenced it anyway.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 797f1d3..2584710 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -1943,7 +1943,7 @@ int gfs2_inplace_reserve(struct gfs2_inode *ip, const struct gfs2_alloc_parms *a
 		/* Skip unuseable resource groups */
 		if ((rs->rs_rbm.rgd->rd_flags & (GFS2_RGF_NOALLOC |
 						 GFS2_RDF_ERROR)) ||
-		    (ap && (ap->target > rs->rs_rbm.rgd->rd_extfail_pt)))
+		    (ap->target > rs->rs_rbm.rgd->rd_extfail_pt))
 			goto skip_rgrp;
 
 		if (sdp->sd_args.ar_rgrplvb)
-- 
1.8.3.1

[Cluster-devel] [gfs2:master 3/5] fs/gfs2/rgrp.c:1946 gfs2_inplace_reserve() warn: variable dereferenced before check 'ap' (see line 1938)

[Dan Carpenter]

On Wed, Dec 04, 2013 at 11:18:01AM +0000, Steven Whitehouse wrote:
> Hi,
> 
> On Tue, 2013-12-03 at 14:01 +0300, Dan Carpenter wrote:
> [snip]
> > > > 90306c41 Benjamin Marzinski 2012-05-29  1939  				}
> > > > 90306c41 Benjamin Marzinski 2012-05-29  1940  			}
> > > > 292c8c14 Abhijith Das       2007-11-29  1941  		}
> > > > 666d1d8a Bob Peterson       2012-06-13  1942  
> > > > c743ffd0 Steven Whitehouse  2012-08-25  1943  		/* Skip unuseable resource groups */
> > > > 2d7e8aac Bob Peterson       2013-11-25  1944  		if ((rs->rs_rbm.rgd->rd_flags & (GFS2_RGF_NOALLOC |
> > > > 2d7e8aac Bob Peterson       2013-11-25  1945  						 GFS2_RDF_ERROR)) ||
> > > > 2d7e8aac Bob Peterson       2013-11-25 @1946  		    (ap && (ap->target > rs->rs_rbm.rgd->rd_extfail_pt)))
> > >  ... and this looks ok to me.
> > > 
> > > Can you give some more detail as to whats wrong here?
> > 
> > The check that was introduced could be removed because this function is
> > always called with a valid "ap" and we dereferenced it already.
> > 
> > regards,
> > dan carpenter
> > 
> 
> Ah, now I see what it is referring to. How does the following look as a
> fix?
> 

Perfect.

regards,
dan carpenter