[PATCH 0/9] xfsprogs: fix build warnings V2

[PATCH 0/9] xfsprogs: fix build warnings V2

[Dave Chinner]

This is the complete set of patches that make xfsprogs build cleanly with gcc
4.3.4 on my debian system. The majority of the warnings are dir2 name string
sign warnings, most of the rest are array subscript bounds warnings which gcc
has got wrong. The only real bug in whole the lot is the missing error check in
xfs_rtfree_range().

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

[PATCH 1/9] xfsprogs: fix sign warnings in libxfs

[Dave Chinner]

The directory naming is a mix of char and uchar_t and the compiler
warns wheter they cross over. Cast the pointers according to the
destination requirements as it doesn't seem to matter to avoid
the warnings.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 include/xfs_types.h     |    2 +-
 libxfs/xfs_attr.c       |   10 +++++-----
 libxfs/xfs_attr_leaf.c  |   10 +++++-----
 libxfs/xfs_dir2.c       |    2 +-
 libxfs/xfs_dir2_block.c |    8 ++++----
 libxfs/xfs_dir2_leaf.c  |    4 ++--
 libxfs/xfs_dir2_node.c  |    4 ++--
 libxfs/xfs_dir2_sf.c    |   13 +++++++------
 8 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/include/xfs_types.h b/include/xfs_types.h
index 0f51916..6f566ff 100644
--- a/include/xfs_types.h
+++ b/include/xfs_types.h
@@ -161,7 +161,7 @@ typedef enum {
 } xfs_btnum_t;
 
 struct xfs_name {
-	const char	*name;
+	const uchar_t	*name;
 	int		len;
 };
 
diff --git a/libxfs/xfs_attr.c b/libxfs/xfs_attr.c
index 7ab37ff..5bd3c2a 100644
--- a/libxfs/xfs_attr.c
+++ b/libxfs/xfs_attr.c
@@ -64,7 +64,7 @@ xfs_attr_name_to_xname(
 {
 	if (!aname)
 		return EINVAL;
-	xname->name = aname;
+	xname->name = (const uchar_t *)aname;
 	xname->len = strlen(aname);
 	if (xname->len >= MAXNAMELEN)
 		return EFAULT;		/* match IRIX behaviour */
@@ -103,7 +103,7 @@ xfs_attr_fetch(xfs_inode_t *ip, struct xfs_name *name,
 	memset((char *)&args, 0, sizeof(args));
 	args.name = name->name;
 	args.namelen = name->len;
-	args.value = value;
+	args.value = (uchar_t *)value;
 	args.valuelen = *valuelenp;
 	args.flags = flags;
 	args.hashval = xfs_da_hashname(args.name, args.namelen);
@@ -233,7 +233,7 @@ xfs_attr_set_int(xfs_inode_t *dp, struct xfs_name *name,
 	memset((char *)&args, 0, sizeof(args));
 	args.name = name->name;
 	args.namelen = name->len;
-	args.value = value;
+	args.value = (uchar_t *)value;
 	args.valuelen = valuelen;
 	args.flags = flags;
 	args.hashval = xfs_da_hashname(args.name, args.namelen);
@@ -1545,7 +1545,7 @@ xfs_attr_rmtval_get(xfs_da_args_t *args)
 	ASSERT(!(args->flags & ATTR_KERNOVAL));
 
 	mp = args->dp->i_mount;
-	dst = args->value;
+	dst = (xfs_caddr_t)args->value;
 	valuelen = args->valuelen;
 	lblkno = args->rmtblkno;
 	while (valuelen > 0) {
@@ -1601,7 +1601,7 @@ xfs_attr_rmtval_set(xfs_da_args_t *args)
 
 	dp = args->dp;
 	mp = dp->i_mount;
-	src = args->value;
+	src = (xfs_caddr_t)args->value;
 
 	/*
 	 * Find a "hole" in the attribute address space large enough for
diff --git a/libxfs/xfs_attr_leaf.c b/libxfs/xfs_attr_leaf.c
index f8f926f..d535752 100644
--- a/libxfs/xfs_attr_leaf.c
+++ b/libxfs/xfs_attr_leaf.c
@@ -476,11 +476,11 @@ xfs_attr_shortform_to_leaf(xfs_da_args_t *args)
 
 	sfe = &sf->list[0];
 	for (i = 0; i < sf->hdr.count; i++) {
-		nargs.name = (char *)sfe->nameval;
+		nargs.name = (uchar_t *)sfe->nameval;
 		nargs.namelen = sfe->namelen;
-		nargs.value = (char *)&sfe->nameval[nargs.namelen];
+		nargs.value = (uchar_t *)&sfe->nameval[nargs.namelen];
 		nargs.valuelen = sfe->valuelen;
-		nargs.hashval = xfs_da_hashname((char *)sfe->nameval,
+		nargs.hashval = xfs_da_hashname((uchar_t *)sfe->nameval,
 						sfe->namelen);
 		nargs.flags = XFS_ATTR_NSP_ONDISK_TO_ARGS(sfe->flags);
 		error = xfs_attr_leaf_lookup_int(bp, &nargs); /* set a->index */
@@ -610,9 +610,9 @@ xfs_attr_leaf_to_shortform(xfs_dabuf_t *bp, xfs_da_args_t *args, int forkoff)
 			continue;
 		ASSERT(entry->flags & XFS_ATTR_LOCAL);
 		name_loc = XFS_ATTR_LEAF_NAME_LOCAL(leaf, i);
-		nargs.name = (char *)name_loc->nameval;
+		nargs.name = (uchar_t *)name_loc->nameval;
 		nargs.namelen = name_loc->namelen;
-		nargs.value = (char *)&name_loc->nameval[nargs.namelen];
+		nargs.value = (uchar_t *)&name_loc->nameval[nargs.namelen];
 		nargs.valuelen = be16_to_cpu(name_loc->valuelen);
 		nargs.hashval = be32_to_cpu(entry->hashval);
 		nargs.flags = XFS_ATTR_NSP_ONDISK_TO_ARGS(entry->flags);
diff --git a/libxfs/xfs_dir2.c b/libxfs/xfs_dir2.c
index 71134f8..2d2abd1 100644
--- a/libxfs/xfs_dir2.c
+++ b/libxfs/xfs_dir2.c
@@ -18,7 +18,7 @@
 
 #include <xfs.h>
 
-struct xfs_name xfs_name_dotdot = {"..", 2};
+struct xfs_name xfs_name_dotdot = {(uchar_t *)"..", 2};
 
 extern const struct xfs_nameops xfs_default_nameops;
 
diff --git a/libxfs/xfs_dir2_block.c b/libxfs/xfs_dir2_block.c
index d197b0b..d286000 100644
--- a/libxfs/xfs_dir2_block.c
+++ b/libxfs/xfs_dir2_block.c
@@ -36,8 +36,8 @@ static xfs_dahash_t xfs_dir_hash_dot, xfs_dir_hash_dotdot;
 void
 xfs_dir_startup(void)
 {
-	xfs_dir_hash_dot = xfs_da_hashname(".", 1);
-	xfs_dir_hash_dotdot = xfs_da_hashname("..", 2);
+	xfs_dir_hash_dot = xfs_da_hashname((uchar_t *)".", 1);
+	xfs_dir_hash_dotdot = xfs_da_hashname((uchar_t *)"..", 2);
 }
 
 /*
@@ -489,7 +489,7 @@ xfs_dir2_block_lookup(
 	 * Fill in inode number, CI name if appropriate, release the block.
 	 */
 	args->inumber = be64_to_cpu(dep->inumber);
-	error = xfs_dir_cilookup_result(args, dep->name, dep->namelen);
+	error = xfs_dir_cilookup_result(args, (char *)dep->name, dep->namelen);
 	xfs_da_brelse(args->trans, bp);
 	return XFS_ERROR(error);
 }
@@ -576,7 +576,7 @@ xfs_dir2_block_lookup_int(
 		 * and buffer. If it's the first case-insensitive match, store
 		 * the index and buffer and continue looking for an exact match.
 		 */
-		cmp = mp->m_dirnameops->compname(args, dep->name, dep->namelen);
+		cmp = mp->m_dirnameops->compname(args, (char *)dep->name, dep->namelen);
 		if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) {
 			args->cmpresult = cmp;
 			*bpp = bp;
diff --git a/libxfs/xfs_dir2_leaf.c b/libxfs/xfs_dir2_leaf.c
index 9a1aace..356b541 100644
--- a/libxfs/xfs_dir2_leaf.c
+++ b/libxfs/xfs_dir2_leaf.c
@@ -906,7 +906,7 @@ xfs_dir2_leaf_lookup(
 	 * Return the found inode number & CI name if appropriate
 	 */
 	args->inumber = be64_to_cpu(dep->inumber);
-	error = xfs_dir_cilookup_result(args, dep->name, dep->namelen);
+	error = xfs_dir_cilookup_result(args, (char *)dep->name, dep->namelen);
 	xfs_da_brelse(tp, dbp);
 	xfs_da_brelse(tp, lbp);
 	return XFS_ERROR(error);
@@ -1000,7 +1000,7 @@ xfs_dir2_leaf_lookup_int(
 		 * and buffer. If it's the first case-insensitive match, store
 		 * the index and buffer and continue looking for an exact match.
 		 */
-		cmp = mp->m_dirnameops->compname(args, dep->name, dep->namelen);
+		cmp = mp->m_dirnameops->compname(args, (char *)dep->name, dep->namelen);
 		if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) {
 			args->cmpresult = cmp;
 			*indexp = index;
diff --git a/libxfs/xfs_dir2_node.c b/libxfs/xfs_dir2_node.c
index db88adc..6a92f13 100644
--- a/libxfs/xfs_dir2_node.c
+++ b/libxfs/xfs_dir2_node.c
@@ -612,7 +612,7 @@ xfs_dir2_leafn_lookup_for_entry(
 		 * EEXIST immediately. If it's the first case-insensitive
 		 * match, store the block & inode number and continue looking.
 		 */
-		cmp = mp->m_dirnameops->compname(args, dep->name, dep->namelen);
+		cmp = mp->m_dirnameops->compname(args, (char *)dep->name, dep->namelen);
 		if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) {
 			/* If there is a CI match block, drop it */
 			if (args->cmpresult != XFS_CMP_DIFFERENT &&
@@ -1821,7 +1821,7 @@ xfs_dir2_node_lookup(
 
 		dep = (xfs_dir2_data_entry_t *)((char *)state->extrablk.bp->
 						data + state->extrablk.index);
-		rval = xfs_dir_cilookup_result(args, dep->name, dep->namelen);
+		rval = xfs_dir_cilookup_result(args, (char *)dep->name, dep->namelen);
 	}
 	/*
 	 * Release the btree blocks and leaf block.
diff --git a/libxfs/xfs_dir2_sf.c b/libxfs/xfs_dir2_sf.c
index 77f30de..06e076d 100644
--- a/libxfs/xfs_dir2_sf.c
+++ b/libxfs/xfs_dir2_sf.c
@@ -732,8 +732,8 @@ xfs_dir2_sf_lookup(
 		 * number. If it's the first case-insensitive match, store the
 		 * inode number and continue looking for an exact match.
 		 */
-		cmp = dp->i_mount->m_dirnameops->compname(args, sfep->name,
-								sfep->namelen);
+		cmp = dp->i_mount->m_dirnameops->compname(args,
+					(char *)sfep->name, sfep->namelen);
 		if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) {
 			args->cmpresult = cmp;
 			args->inumber = xfs_dir2_sf_get_inumber(sfp,
@@ -751,7 +751,8 @@ xfs_dir2_sf_lookup(
 	if (!ci_sfep)
 		return XFS_ERROR(ENOENT);
 	/* otherwise process the CI match as required by the caller */
-	error = xfs_dir_cilookup_result(args, ci_sfep->name, ci_sfep->namelen);
+	error = xfs_dir_cilookup_result(args, (char *)ci_sfep->name,
+						ci_sfep->namelen);
 	return XFS_ERROR(error);
 }
 
@@ -793,7 +794,7 @@ xfs_dir2_sf_removename(
 	 */
 	for (i = 0, sfep = xfs_dir2_sf_firstentry(sfp); i < sfp->hdr.count;
 				i++, sfep = xfs_dir2_sf_nextentry(sfp, sfep)) {
-		if (xfs_da_compname(args, sfep->name, sfep->namelen) ==
+		if (xfs_da_compname(args, (char *)sfep->name, sfep->namelen) ==
 								XFS_CMP_EXACT) {
 			ASSERT(xfs_dir2_sf_get_inumber(sfp,
 						xfs_dir2_sf_inumberp(sfep)) ==
@@ -928,8 +929,8 @@ xfs_dir2_sf_replace(
 		for (i = 0, sfep = xfs_dir2_sf_firstentry(sfp);
 				i < sfp->hdr.count;
 				i++, sfep = xfs_dir2_sf_nextentry(sfp, sfep)) {
-			if (xfs_da_compname(args, sfep->name, sfep->namelen) ==
-								XFS_CMP_EXACT) {
+			if (xfs_da_compname(args, (char *)sfep->name,
+					sfep->namelen) == XFS_CMP_EXACT) {
 #if XFS_BIG_INUMS || defined(DEBUG)
 				ino = xfs_dir2_sf_get_inumber(sfp,
 					xfs_dir2_sf_inumberp(sfep));
-- 
1.6.5

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

Re: [PATCH 1/9] xfsprogs: fix sign warnings in libxfs

[Christoph Hellwig]

On Thu, Jan 14, 2010 at 09:09:04PM +1100, Dave Chinner wrote:
> The directory naming is a mix of char and uchar_t and the compiler
> warns wheter they cross over. Cast the pointers according to the
> destination requirements as it doesn't seem to matter to avoid
> the warnings.

I think this makes sense, but it should really go into the kernel
code first - we currently paper over the warnings there by using
-funsigned-char for all XFS code which we might be able to remove
with those changes.

>  
>  struct xfs_name {
> -	const char	*name;
> +	const uchar_t	*name;
>  	int		len;

Maye use on of the standard types (unsigned char / u8 / uint8_t)
instead of the weird xfs-specific uchar_t, which in fact is already
gone in the kernel?

> @@ -1545,7 +1545,7 @@ xfs_attr_rmtval_get(xfs_da_args_t *args)
>  	ASSERT(!(args->flags & ATTR_KERNOVAL));
>  
>  	mp = args->dp->i_mount;
> -	dst = args->value;
> +	dst = (xfs_caddr_t)args->value;
>  	valuelen = args->valuelen;
>  	lblkno = args->rmtblkno;
>  	while (valuelen > 0) {
> @@ -1601,7 +1601,7 @@ xfs_attr_rmtval_set(xfs_da_args_t *args)
>  
>  	dp = args->dp;
>  	mp = dp->i_mount;
> -	src = args->value;
> +	src = (xfs_caddr_t)args->value;

If we make xfs_buf_iomove take a void * we can just pass on the
unsignedness in those two places.

> diff --git a/libxfs/xfs_attr_leaf.c b/libxfs/xfs_attr_leaf.c
> index f8f926f..d535752 100644
> --- a/libxfs/xfs_attr_leaf.c
> +++ b/libxfs/xfs_attr_leaf.c
> @@ -476,11 +476,11 @@ xfs_attr_shortform_to_leaf(xfs_da_args_t *args)
>  
>  	sfe = &sf->list[0];
>  	for (i = 0; i < sf->hdr.count; i++) {
> -		nargs.name = (char *)sfe->nameval;
> +		nargs.name = (uchar_t *)sfe->nameval;
>  		nargs.namelen = sfe->namelen;
> -		nargs.value = (char *)&sfe->nameval[nargs.namelen];
> +		nargs.value = (uchar_t *)&sfe->nameval[nargs.namelen];
>  		nargs.valuelen = sfe->valuelen;
> -		nargs.hashval = xfs_da_hashname((char *)sfe->nameval,
> +		nargs.hashval = xfs_da_hashname((uchar_t *)sfe->nameval,
>  						sfe->namelen);

Both the leaf and sf nameval fields already are __u8/__uint8_t aka
unsigned char, so I can't see why can't see why any casts are needed
here and in the later hunks dealing with namevals.

> @@ -489,7 +489,7 @@ xfs_dir2_block_lookup(
>  	 * Fill in inode number, CI name if appropriate, release the block.
>  	 */
>  	args->inumber = be64_to_cpu(dep->inumber);
> -	error = xfs_dir_cilookup_result(args, dep->name, dep->namelen);
> +	error = xfs_dir_cilookup_result(args, (char *)dep->name, dep->namelen);

xfs_dir_cilookup_result always gets unsigned char names now, so I'd move
the conversion into it.

>  	xfs_da_brelse(args->trans, bp);
>  	return XFS_ERROR(error);
>  }
> @@ -576,7 +576,7 @@ xfs_dir2_block_lookup_int(
>  		 * and buffer. If it's the first case-insensitive match, store
>  		 * the index and buffer and continue looking for an exact match.
>  		 */
> -		cmp = mp->m_dirnameops->compname(args, dep->name, dep->namelen);
> +		cmp = mp->m_dirnameops->compname(args, (char *)dep->name, dep->namelen);

Also here, ->compname appears to always get unsigned names.

> -		if (xfs_da_compname(args, sfep->name, sfep->namelen) ==
> +		if (xfs_da_compname(args, (char *)sfep->name, sfep->namelen) ==
>  								XFS_CMP_EXACT) {
>  			ASSERT(xfs_dir2_sf_get_inumber(sfp,
>  						xfs_dir2_sf_inumberp(sfep)) ==
> @@ -928,8 +929,8 @@ xfs_dir2_sf_replace(
>  		for (i = 0, sfep = xfs_dir2_sf_firstentry(sfp);
>  				i < sfp->hdr.count;
>  				i++, sfep = xfs_dir2_sf_nextentry(sfp, sfep)) {
> -			if (xfs_da_compname(args, sfep->name, sfep->namelen) ==
> -								XFS_CMP_EXACT) {
> +			if (xfs_da_compname(args, (char *)sfep->name,
> +					sfep->namelen) == XFS_CMP_EXACT) {

Same seems to apply for direct calls to xfs_da_compname.

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

[PATCH 2/9] xfsprogs: Fix miscellaneous warnings in libxfs

[Dave Chinner]

These are questionable fixes but they do shut up the warnings with
as little change as possible. The first couple are questionable
because they are a result of the compiler (gcc 4.3.4) trying to be
smart but failing.

The uninitialised variable warnings in xfs_bmapi() is spurious, as
certain fields are unused in certain situations but gcc can't work
that out.  It is easily suppressed by initialising the structure to
zero.

xfs_mod_sb() is dumping out an array subscript bound warning that is
spurious. The code is correct and will assert fail on out of bounds
indexes. A simple reorder of the first and last offset calculations
makes the warning go away (a good sign of compiler fail if ever
there was one).

Changing xfs_alloc_mark_busy() from a macro to a static inline
definition is needed for the compiler to see that variables passed
to that function are actually used.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 libxfs/xfs.h       |    5 +++--
 libxfs/xfs_bmap.c  |    2 +-
 libxfs/xfs_mount.c |    9 ++++-----
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/libxfs/xfs.h b/libxfs/xfs.h
index 8e94dad..abe8700 100644
--- a/libxfs/xfs.h
+++ b/libxfs/xfs.h
@@ -288,9 +288,10 @@ roundup_pow_of_two(uint v)
 #define xfs_ilock(ip,mode)				((void) 0)
 #define xfs_iunlock(ip,mode)				((void) 0)
 
-/* space allocation */
+/* space allocation - use static inlines to avoid unused var warnings */
 #define xfs_alloc_search_busy(tp,ag,b,len)	((void) 0)
-#define xfs_alloc_mark_busy(tp,ag,b,len)	((void) 0)
+static inline void xfs_alloc_mark_busy(xfs_trans_t *tp,
+	xfs_agnumber_t agno, xfs_agblock_t bno, xfs_extlen_t len) {}
 #define xfs_rotorstep				1
 #define xfs_bmap_rtalloc(a)			(ENOSYS)
 #define xfs_rtpick_extent(mp,tp,len,p)		(ENOSYS)
diff --git a/libxfs/xfs_bmap.c b/libxfs/xfs_bmap.c
index cf1123a..4cc6431 100644
--- a/libxfs/xfs_bmap.c
+++ b/libxfs/xfs_bmap.c
@@ -4274,7 +4274,7 @@ xfs_bmapi(
 	xfs_fsblock_t	abno;		/* allocated block number */
 	xfs_extlen_t	alen;		/* allocated extent length */
 	xfs_fileoff_t	aoff;		/* allocated file offset */
-	xfs_bmalloca_t	bma;		/* args for xfs_bmap_alloc */
+	xfs_bmalloca_t	bma = { 0 };	/* args for xfs_bmap_alloc */
 	xfs_btree_cur_t	*cur;		/* bmap btree cursor */
 	xfs_fileoff_t	end;		/* end of mapped file region */
 	int		eof;		/* we've hit the end of extents */
diff --git a/libxfs/xfs_mount.c b/libxfs/xfs_mount.c
index 02bff42..662c8d2 100644
--- a/libxfs/xfs_mount.c
+++ b/libxfs/xfs_mount.c
@@ -400,15 +400,14 @@ xfs_mod_sb(xfs_trans_t *tp, __int64_t fields)
 
 	xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb, fields);
 
-	/* find modified range */
+	/* find modified range - get last first to avoid spurious warning */
+	f = (xfs_sb_field_t)xfs_highbit64((__uint64_t)fields);
+	ASSERT((1LL << f) & XFS_SB_MOD_BITS);
+	last = xfs_sb_info[f + 1].offset - 1;
 
 	f = (xfs_sb_field_t)xfs_lowbit64((__uint64_t)fields);
 	ASSERT((1LL << f) & XFS_SB_MOD_BITS);
 	first = xfs_sb_info[f].offset;
 
-	f = (xfs_sb_field_t)xfs_highbit64((__uint64_t)fields);
-	ASSERT((1LL << f) & XFS_SB_MOD_BITS);
-	last = xfs_sb_info[f + 1].offset - 1;
-
 	xfs_trans_log_buf(tp, bp, first, last);
 }
-- 
1.6.5

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

Re: [PATCH 2/9] xfsprogs: Fix miscellaneous warnings in libxfs

[Christoph Hellwig]

> -/* space allocation */
> +/* space allocation - use static inlines to avoid unused var warnings */
>  #define xfs_alloc_search_busy(tp,ag,b,len)	((void) 0)
> -#define xfs_alloc_mark_busy(tp,ag,b,len)	((void) 0)
> +static inline void xfs_alloc_mark_busy(xfs_trans_t *tp,
> +	xfs_agnumber_t agno, xfs_agblock_t bno, xfs_extlen_t len) {}
>  #define xfs_rotorstep				1
>  #define xfs_bmap_rtalloc(a)			(ENOSYS)
>  #define xfs_rtpick_extent(mp,tp,len,p)		(ENOSYS)

Maybe just convert all these to inlines so that we make sure all
arguments are evaluated?

> diff --git a/libxfs/xfs_bmap.c b/libxfs/xfs_bmap.c
> index cf1123a..4cc6431 100644
> --- a/libxfs/xfs_bmap.c
> +++ b/libxfs/xfs_bmap.c
> @@ -4274,7 +4274,7 @@ xfs_bmapi(
>  	xfs_fsblock_t	abno;		/* allocated block number */
>  	xfs_extlen_t	alen;		/* allocated extent length */
>  	xfs_fileoff_t	aoff;		/* allocated file offset */
> -	xfs_bmalloca_t	bma;		/* args for xfs_bmap_alloc */
> +	xfs_bmalloca_t	bma = { 0 };	/* args for xfs_bmap_alloc */
>  	xfs_btree_cur_t	*cur;		/* bmap btree cursor */
>  	xfs_fileoff_t	end;		/* end of mapped file region */
>  	int		eof;		/* we've hit the end of extents */

Not nice but okay..

> diff --git a/libxfs/xfs_mount.c b/libxfs/xfs_mount.c
> index 02bff42..662c8d2 100644
> --- a/libxfs/xfs_mount.c
> +++ b/libxfs/xfs_mount.c
> @@ -400,15 +400,14 @@ xfs_mod_sb(xfs_trans_t *tp, __int64_t fields)
>  
>  	xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb, fields);
>  
> -	/* find modified range */
> +	/* find modified range - get last first to avoid spurious warning */
> +	f = (xfs_sb_field_t)xfs_highbit64((__uint64_t)fields);
> +	ASSERT((1LL << f) & XFS_SB_MOD_BITS);
> +	last = xfs_sb_info[f + 1].offset - 1;
>  
>  	f = (xfs_sb_field_t)xfs_lowbit64((__uint64_t)fields);
>  	ASSERT((1LL << f) & XFS_SB_MOD_BITS);
>  	first = xfs_sb_info[f].offset;
>  
> -	f = (xfs_sb_field_t)xfs_highbit64((__uint64_t)fields);
> -	ASSERT((1LL << f) & XFS_SB_MOD_BITS);
> -	last = xfs_sb_info[f + 1].offset - 1;
> -

I don't see how this can shut up a warning, but as it doesn't cause harm
I'm fine with it.  But this and the above one really need to go in via
the kernel tree first.

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

[PATCH 3/9] xfsprogs: fix missing error check in xfs_rtfree_range in libxfs

[Dave Chinner]

When xfs_rtfind_forw() returns an error, the block is returned
uninitialised.  xfs_rtfree_range() is not checking the error return,
so could be using an uninitialised block number for modifying bitmap
summary info.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 libxfs/xfs_rtalloc.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/libxfs/xfs_rtalloc.c b/libxfs/xfs_rtalloc.c
index 6c3da52..4fbdaa9 100644
--- a/libxfs/xfs_rtalloc.c
+++ b/libxfs/xfs_rtalloc.c
@@ -466,6 +466,8 @@ xfs_rtfree_range(
 	 */
 	error = xfs_rtfind_forw(mp, tp, end, mp->m_sb.sb_rextents - 1,
 		&postblock);
+	if (error)
+		return error;
 	/*
 	 * If there are blocks not being freed at the front of the
 	 * old extent, add summary data for them to be allocated.
-- 
1.6.5

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

Re: [PATCH 3/9] xfsprogs: fix missing error check in xfs_rtfree_range in libxfs

[Christoph Hellwig]

On Thu, Jan 14, 2010 at 09:09:06PM +1100, Dave Chinner wrote:
> When xfs_rtfind_forw() returns an error, the block is returned
> uninitialised.  xfs_rtfree_range() is not checking the error return,
> so could be using an uninitialised block number for modifying bitmap
> summary info.

Looks good, (actually a merge of the kernel side fix that just went in)


Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[PATCH 4/9] xfsprogs: fix warning in adfs superblock probe

[Dave Chinner]

The probe gets an array subscript warning because gcc is not smart
enough to realise that a structure made up of multiple byte arrays
in it can be referenced as a flat buffer and it is valid to access
bytes beyond the first array in the structure....

Fix it by passing the adfs superblock in and using the internal
checksum array to get the checksum value.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 libdisk/fstype.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/libdisk/fstype.c b/libdisk/fstype.c
index f84b4e4..548f297 100644
--- a/libdisk/fstype.c
+++ b/libdisk/fstype.c
@@ -142,16 +142,16 @@ may_be_swap(const char *s) {
 
 /* rather weak necessary condition */
 static int
-may_be_adfs(const char *s) {
+may_be_adfs(const struct adfs_super_block *sb) {
 	char *p;
 	int sum;
 
-	p = (char *) s + 511;
+	p = (char *)sb->s_checksum;
 	sum = 0;
-	while(--p != s)
+	while(--p != (char *)sb)
 		sum = (sum >> 8) + (sum & 0xff) + *p;
 
-	return (sum == p[511]);
+	return (sum & 0xff) == sb->s_checksum[0];
 }
 
 static int is_reiserfs_magic_string (struct reiserfs_super_block * rs)
@@ -304,7 +304,7 @@ fstype(const char *device) {
              goto io_error;
 
 	/* only a weak test */
-        if (may_be_adfs((char *) &adfssb)
+        if (may_be_adfs(&adfssb)
             && (adfsblksize(adfssb) >= 8 &&
                 adfsblksize(adfssb) <= 10))
              type = "adfs";
-- 
1.6.5

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

Re: [PATCH 4/9] xfsprogs: fix warning in adfs superblock probe

[Christoph Hellwig]

On Thu, Jan 14, 2010 at 09:09:07PM +1100, Dave Chinner wrote:
> The probe gets an array subscript warning because gcc is not smart
> enough to realise that a structure made up of multiple byte arrays
> in it can be referenced as a flat buffer and it is valid to access
> bytes beyond the first array in the structure....
> 
> Fix it by passing the adfs superblock in and using the internal
> checksum array to get the checksum value.

The check still seems horribly ugly, but hey given that this is code
superceeded by blkid it should be good enough,


Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[PATCH 5/9] xfsprogs: fix some trivial warnings in xfs_db

[Dave Chinner]

Three warnings, none harmful - one dir2 name sign warning and
two cases where gcc can't work out if a variable is initialised
correctly in a child function or not.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 db/check.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/db/check.c b/db/check.c
index 7620d9c..f94a16b 100644
--- a/db/check.c
+++ b/db/check.c
@@ -2317,7 +2317,7 @@ process_data_dir_v2(
 		tag_err += be16_to_cpu(*tagp) != (char *)dep - (char *)data;
 		addr = xfs_dir2_db_off_to_dataptr(mp, db,
 			(char *)dep - (char *)data);
-		xname.name = (char *)dep->name;
+		xname.name = (uchar_t *)dep->name;
 		xname.len = dep->namelen;
 		dir_hash_add(mp->m_dirnameops->hashname(&xname), addr);
 		ptr += xfs_dir2_data_entsize(dep->namelen);
@@ -3059,7 +3059,7 @@ process_leaf_node_dir_v2(
 	xfs_ino_t		lino;
 	int			nex;
 	xfs_ino_t		parent;
-	int			t;
+	int			t = 0;
 	int			v;
 	int			v2;
 	int			x;
@@ -3403,7 +3403,7 @@ process_quota(
 	xfs_fileoff_t	qbno;
 	char		*s = NULL;
 	int		scicb;
-	int		t;
+	int		t = 0;
 
 	switch (qtype) {
 	case IS_USER_QUOTA:
-- 
1.6.5

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

Re: [PATCH 5/9] xfsprogs: fix some trivial warnings in xfs_db

[Christoph Hellwig]

On Thu, Jan 14, 2010 at 09:09:08PM +1100, Dave Chinner wrote:
> Three warnings, none harmful - one dir2 name sign warning and
> two cases where gcc can't work out if a variable is initialised
> correctly in a child function or not.

Might be worth to throw all the dabtree sign things in with the mign
signed issue patch?

Anyway, the changes look good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[PATCH 6/9] xfsprogs: fix trivial warnings in xfs_fsr

[Dave Chinner]

GCC complains about assignments between (unsigned long long *)
and (__u64 *) pointers. Just add a cast to shut it up.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fsr/xfs_fsr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 21f08c3..1f933c7 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -133,7 +133,7 @@ xfs_bulkstat_single(int fd, xfs_ino_t *lastip, xfs_bstat_t *ubuffer)
 {
     xfs_fsop_bulkreq_t  bulkreq;
 
-    bulkreq.lastip = lastip;
+    bulkreq.lastip = (__u64 *)lastip;
     bulkreq.icount = 1;
     bulkreq.ubuffer = ubuffer;
     bulkreq.ocount = NULL;
@@ -146,7 +146,7 @@ xfs_bulkstat(int fd, xfs_ino_t *lastip, int icount,
 {
     xfs_fsop_bulkreq_t  bulkreq;
 
-    bulkreq.lastip = lastip;
+    bulkreq.lastip = (__u64 *)lastip;
     bulkreq.icount = icount;
     bulkreq.ubuffer = ubuffer;
     bulkreq.ocount = ocount;
-- 
1.6.5

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

Re: [PATCH 6/9] xfsprogs: fix trivial warnings in xfs_fsr

[Christoph Hellwig]

On Thu, Jan 14, 2010 at 09:09:09PM +1100, Dave Chinner wrote:
> GCC complains about assignments between (unsigned long long *)
> and (__u64 *) pointers. Just add a cast to shut it up.
> 
> Signed-off-by: Dave Chinner <david@fromorbit.com>

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[PATCH 7/9] xfsprogs: fix print format warnings in xfs_io

[Dave Chinner]

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 io/madvise.c |    2 +-
 io/mincore.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/io/madvise.c b/io/madvise.c
index cd16a4c..3235402 100644
--- a/io/madvise.c
+++ b/io/madvise.c
@@ -96,7 +96,7 @@ madvise_f(
 			return 0;
 		} else if (llength > (size_t)llength) {
 			printf(_("length argument too large -- %lld\n"),
-				llength);
+				(long long)llength);
 			return 0;
 		} else
 			length = (size_t)llength;
diff --git a/io/mincore.c b/io/mincore.c
index d534540..60625ef 100644
--- a/io/mincore.c
+++ b/io/mincore.c
@@ -56,7 +56,7 @@ mincore_f(
 			return 0;
 		} else if (llength > (size_t)llength) {
 			printf(_("length argument too large -- %lld\n"),
-				llength);
+				(long long)llength);
 			return 0;
 		} else
 			length = (size_t)llength;
-- 
1.6.5

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

Re: [PATCH 7/9] xfsprogs: fix print format warnings in xfs_io

[Christoph Hellwig]

On Thu, Jan 14, 2010 at 09:09:10PM +1100, Dave Chinner wrote:
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> ---
>  io/madvise.c |    2 +-
>  io/mincore.c |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[PATCH 8/9] xfsprogs: fix sign warning in mkfs directory code

[Dave Chinner]

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 mkfs/proto.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mkfs/proto.c b/mkfs/proto.c
index 8fc8c4c..3723685 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -432,7 +432,7 @@ parseproto(
 	mode |= val;
 	creds.cr_uid = (int)getnum(pp);
 	creds.cr_gid = (int)getnum(pp);
-	xname.name = name;
+	xname.name = (uchar_t *)name;
 	xname.len = name ? strlen(name) : 0;
 	tp = libxfs_trans_alloc(mp, 0);
 	flags = XFS_ILOG_CORE;
-- 
1.6.5

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

Re: [PATCH 8/9] xfsprogs: fix sign warning in mkfs directory code

[Christoph Hellwig]

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[PATCH 9/9] xfsprogs: fix build warnings in repair

[Dave Chinner]

Rewrite the loop in btree_get_prev() so that the compiler
can see that it returns if the cur->index is zero so it
doesn't complain about possible array bound underflows
when getting the key out of the buffer.

Fix the directory name sign warnings by casting to (uchar_t *)
appropriately.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 repair/btree.c  |    9 +++++----
 repair/phase6.c |   10 +++++-----
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/repair/btree.c b/repair/btree.c
index 3141388..b53b4e4 100644
--- a/repair/btree.c
+++ b/repair/btree.c
@@ -194,12 +194,13 @@ btree_get_prev(
 	}
 
 	/* else need to go up and back down the tree to find the previous */
-
-	while (cur->index == 0) {
-		if (++level == root->height)
-			return NULL;
+	while (++level < root->height) {
+		if (cur->index)
+			break;
 		cur++;
 	}
+	if (cur->index == 0)
+		return NULL;
 
 	/* the key is in the current level */
 	if (key)
diff --git a/repair/phase6.c b/repair/phase6.c
index d056063..884ad62 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -35,7 +35,7 @@ static struct cred		zerocr;
 static struct fsxattr 		zerofsx;
 static xfs_ino_t		orphanage_ino;
 
-static struct xfs_name 		xfs_name_dot = {".", 1};
+static struct xfs_name 		xfs_name_dot = {(uchar_t *)".", 1};
 
 /*
  * Data structures used to keep track of directories where the ".."
@@ -145,7 +145,7 @@ dir_hash_add(
 
 	ASSERT(!hashtab->names_duped);
 
-	xname.name = name;
+	xname.name = (uchar_t *)name;
 	xname.len = namelen;
 
 	junk = name[0] == '/';
@@ -355,7 +355,7 @@ dir_hash_dup_names(dir_hash_tab_t *hashtab)
 	for (p = hashtab->first; p; p = p->nextbyorder) {
 		name = malloc(p->name.len);
 		memcpy(name, p->name.name, p->name.len);
-		p->name.name = name;
+		p->name.name = (uchar_t *)name;
 	}
 	hashtab->names_duped = 1;
 }
@@ -843,7 +843,7 @@ mk_orphanage(xfs_mount_t *mp)
 		do_error(_("%d - couldn't iget root inode to obtain %s\n"),
 			i, ORPHANAGE);
 
-	xname.name = ORPHANAGE;
+	xname.name = (uchar_t *)ORPHANAGE;
 	xname.len = strlen(ORPHANAGE);
 	if (libxfs_dir_lookup(NULL, pip, &xname, &ino, NULL) == 0)
 		return ino;
@@ -944,7 +944,7 @@ mv_orphanage(
 
 	ASSERT(xfs_sb_version_hasdirv2(&mp->m_sb));
 
-	xname.name = fname;
+	xname.name = (uchar_t *)fname;
 	xname.len = snprintf(fname, sizeof(fname), "%llu",
 				(unsigned long long)ino);
 
-- 
1.6.5

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

Re: [PATCH 9/9] xfsprogs: fix build warnings in repair

[Dave Chinner]

On Thu, Jan 14, 2010 at 09:09:12PM +1100, Dave Chinner wrote:
> Rewrite the loop in btree_get_prev() so that the compiler
> can see that it returns if the cur->index is zero so it
> doesn't complain about possible array bound underflows
> when getting the key out of the buffer.
> 
> Fix the directory name sign warnings by casting to (uchar_t *)
> appropriately.
> 
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> ---
>  repair/btree.c  |    9 +++++----
>  repair/phase6.c |   10 +++++-----
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/repair/btree.c b/repair/btree.c
> index 3141388..b53b4e4 100644
> --- a/repair/btree.c
> +++ b/repair/btree.c
> @@ -194,12 +194,13 @@ btree_get_prev(
>  	}
>  
>  	/* else need to go up and back down the tree to find the previous */
> -
> -	while (cur->index == 0) {
> -		if (++level == root->height)
> -			return NULL;
> +	while (++level < root->height) {
> +		if (cur->index)
> +			break;
>  		cur++;
>  	}
> +	if (cur->index == 0)
> +		return NULL;

This is broken - causes a segv. i have a fixed patch, which I'll
send out once I've got xfstests running again on my test box (using
dash as /bin/sh breaks *lots* of stuff in xfstests).

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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