[PATCH] db: fix unsigned char related warnings

[PATCH] db: fix unsigned char related warnings

[Christoph Hellwig]

Clean up the code in hash.c to use the normal char type for all
high-level code, only casting to uint8_t when calling into low-level
code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 db/hash.c | 45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/db/hash.c b/db/hash.c
index 716da88ba..05a94f249 100644
--- a/db/hash.c
+++ b/db/hash.c
@@ -65,16 +65,15 @@ hash_f(
 	}
 
 	for (c = optind; c < argc; c++) {
-		if (use_dir2_hash) {
-			struct xfs_name	xname = {
-				.name	= (uint8_t *)argv[c],
-				.len	= strlen(argv[c]),
-			};
+		struct xfs_name	xname = {
+			.name	= (uint8_t *)argv[c],
+			.len	= strlen(argv[c]),
+		};
 
+		if (use_dir2_hash)
 			hashval = libxfs_dir2_hashname(mp, &xname);
-		} else {
-			hashval = libxfs_da_hashname(argv[c], strlen(argv[c]));
-		}
+		else
+			hashval = libxfs_da_hashname(xname.name, xname.len);
 		dbprintf("0x%x\n", hashval);
 	}
 
@@ -103,7 +102,7 @@ struct name_dup {
 	struct name_dup	*next;
 	uint32_t	crc;
 	uint8_t		namelen;
-	uint8_t		name[];
+	char		name[];
 };
 
 static inline size_t
@@ -175,7 +174,7 @@ dup_table_free(
 static struct name_dup *
 dup_table_find(
 	struct dup_table	*tab,
-	unsigned char		*name,
+	char			*name,
 	size_t			namelen)
 {
 	struct name_dup		*ent;
@@ -197,7 +196,7 @@ dup_table_find(
 static int
 dup_table_store(
 	struct dup_table	*tab,
-	unsigned char		*name,
+	char			*name,
 	size_t			namelen)
 {
 	struct name_dup		*dup;
@@ -209,7 +208,7 @@ dup_table_store(
 		int		ret;
 
 		do {
-			ret = find_alternate(namelen, name, seq++);
+			ret = find_alternate(namelen, (uint8_t *)name, seq++);
 		} while (ret == 0);
 		if (ret < 0)
 			return EEXIST;
@@ -231,15 +230,15 @@ dup_table_store(
 static int
 collide_dirents(
 	unsigned long		nr,
-	const unsigned char	*name,
+	char			*name,
 	size_t			namelen,
 	int			fd)
 {
 	struct xfs_name		dname = {
-		.name		= name,
+		.name		= (uint8_t *)name,
 		.len		= namelen,
 	};
-	unsigned char		direntname[MAXNAMELEN + 1];
+	char			direntname[MAXNAMELEN + 1];
 	struct dup_table	*tab = NULL;
 	xfs_dahash_t		old_hash;
 	unsigned long		i;
@@ -268,10 +267,10 @@ collide_dirents(
 			return error;
 	}
 
-	dname.name = direntname;
+	dname.name = (uint8_t *)direntname;
 	for (i = 0; i < nr; i++) {
 		strncpy(direntname, name, MAXNAMELEN);
-		obfuscate_name(old_hash, namelen, direntname, true);
+		obfuscate_name(old_hash, namelen, (uint8_t *)direntname, true);
 		ASSERT(old_hash == libxfs_dir2_hashname(mp, &dname));
 
 		if (fd >= 0) {
@@ -297,17 +296,17 @@ collide_dirents(
 static int
 collide_xattrs(
 	unsigned long		nr,
-	const unsigned char	*name,
+	char			*name,
 	size_t			namelen,
 	int			fd)
 {
-	unsigned char		xattrname[MAXNAMELEN + 5];
+	char			xattrname[MAXNAMELEN + 5];
 	struct dup_table	*tab = NULL;
 	xfs_dahash_t		old_hash;
 	unsigned long		i;
 	int			error;
 
-	old_hash = libxfs_da_hashname(name, namelen);
+	old_hash = libxfs_da_hashname((uint8_t *)name, namelen);
 
 	if (fd >= 0) {
 		/*
@@ -330,8 +329,10 @@ collide_xattrs(
 
 	for (i = 0; i < nr; i++) {
 		snprintf(xattrname, MAXNAMELEN + 5, "user.%s", name);
-		obfuscate_name(old_hash, namelen, xattrname + 5, false);
-		ASSERT(old_hash == libxfs_da_hashname(xattrname + 5, namelen));
+		obfuscate_name(old_hash, namelen, (uint8_t *)xattrname + 5,
+				false);
+		ASSERT(old_hash == libxfs_da_hashname((uint8_t *)xattrname + 5,
+				namelen));
 
 		if (fd >= 0) {
 			error = fsetxattr(fd, xattrname, "1", 1, 0);
-- 
2.39.2

Re: [PATCH] db: fix unsigned char related warnings

[Darrick J. Wong]

On Fri, Nov 03, 2023 at 05:02:10PM +0100, Christoph Hellwig wrote:
> Clean up the code in hash.c to use the normal char type for all
> high-level code, only casting to uint8_t when calling into low-level
> code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

The problem is deeper than just this, but we gotta start somewhere...

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  db/hash.c | 45 +++++++++++++++++++++++----------------------
>  1 file changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/db/hash.c b/db/hash.c
> index 716da88ba..05a94f249 100644
> --- a/db/hash.c
> +++ b/db/hash.c
> @@ -65,16 +65,15 @@ hash_f(
>  	}
>  
>  	for (c = optind; c < argc; c++) {
> -		if (use_dir2_hash) {
> -			struct xfs_name	xname = {
> -				.name	= (uint8_t *)argv[c],
> -				.len	= strlen(argv[c]),
> -			};
> +		struct xfs_name	xname = {
> +			.name	= (uint8_t *)argv[c],
> +			.len	= strlen(argv[c]),
> +		};
>  
> +		if (use_dir2_hash)
>  			hashval = libxfs_dir2_hashname(mp, &xname);
> -		} else {
> -			hashval = libxfs_da_hashname(argv[c], strlen(argv[c]));
> -		}
> +		else
> +			hashval = libxfs_da_hashname(xname.name, xname.len);
>  		dbprintf("0x%x\n", hashval);
>  	}
>  
> @@ -103,7 +102,7 @@ struct name_dup {
>  	struct name_dup	*next;
>  	uint32_t	crc;
>  	uint8_t		namelen;
> -	uint8_t		name[];
> +	char		name[];
>  };
>  
>  static inline size_t
> @@ -175,7 +174,7 @@ dup_table_free(
>  static struct name_dup *
>  dup_table_find(
>  	struct dup_table	*tab,
> -	unsigned char		*name,
> +	char			*name,
>  	size_t			namelen)
>  {
>  	struct name_dup		*ent;
> @@ -197,7 +196,7 @@ dup_table_find(
>  static int
>  dup_table_store(
>  	struct dup_table	*tab,
> -	unsigned char		*name,
> +	char			*name,
>  	size_t			namelen)
>  {
>  	struct name_dup		*dup;
> @@ -209,7 +208,7 @@ dup_table_store(
>  		int		ret;
>  
>  		do {
> -			ret = find_alternate(namelen, name, seq++);
> +			ret = find_alternate(namelen, (uint8_t *)name, seq++);
>  		} while (ret == 0);
>  		if (ret < 0)
>  			return EEXIST;
> @@ -231,15 +230,15 @@ dup_table_store(
>  static int
>  collide_dirents(
>  	unsigned long		nr,
> -	const unsigned char	*name,
> +	char			*name,
>  	size_t			namelen,
>  	int			fd)
>  {
>  	struct xfs_name		dname = {
> -		.name		= name,
> +		.name		= (uint8_t *)name,
>  		.len		= namelen,
>  	};
> -	unsigned char		direntname[MAXNAMELEN + 1];
> +	char			direntname[MAXNAMELEN + 1];
>  	struct dup_table	*tab = NULL;
>  	xfs_dahash_t		old_hash;
>  	unsigned long		i;
> @@ -268,10 +267,10 @@ collide_dirents(
>  			return error;
>  	}
>  
> -	dname.name = direntname;
> +	dname.name = (uint8_t *)direntname;
>  	for (i = 0; i < nr; i++) {
>  		strncpy(direntname, name, MAXNAMELEN);
> -		obfuscate_name(old_hash, namelen, direntname, true);
> +		obfuscate_name(old_hash, namelen, (uint8_t *)direntname, true);
>  		ASSERT(old_hash == libxfs_dir2_hashname(mp, &dname));
>  
>  		if (fd >= 0) {
> @@ -297,17 +296,17 @@ collide_dirents(
>  static int
>  collide_xattrs(
>  	unsigned long		nr,
> -	const unsigned char	*name,
> +	char			*name,
>  	size_t			namelen,
>  	int			fd)
>  {
> -	unsigned char		xattrname[MAXNAMELEN + 5];
> +	char			xattrname[MAXNAMELEN + 5];
>  	struct dup_table	*tab = NULL;
>  	xfs_dahash_t		old_hash;
>  	unsigned long		i;
>  	int			error;
>  
> -	old_hash = libxfs_da_hashname(name, namelen);
> +	old_hash = libxfs_da_hashname((uint8_t *)name, namelen);
>  
>  	if (fd >= 0) {
>  		/*
> @@ -330,8 +329,10 @@ collide_xattrs(
>  
>  	for (i = 0; i < nr; i++) {
>  		snprintf(xattrname, MAXNAMELEN + 5, "user.%s", name);
> -		obfuscate_name(old_hash, namelen, xattrname + 5, false);
> -		ASSERT(old_hash == libxfs_da_hashname(xattrname + 5, namelen));
> +		obfuscate_name(old_hash, namelen, (uint8_t *)xattrname + 5,
> +				false);
> +		ASSERT(old_hash == libxfs_da_hashname((uint8_t *)xattrname + 5,
> +				namelen));
>  
>  		if (fd >= 0) {
>  			error = fsetxattr(fd, xattrname, "1", 1, 0);
> -- 
> 2.39.2
> 

Re: [PATCH] db: fix unsigned char related warnings

[Christoph Hellwig]

On Fri, Nov 03, 2023 at 01:38:13PM -0700, Darrick J. Wong wrote:
> On Fri, Nov 03, 2023 at 05:02:10PM +0100, Christoph Hellwig wrote:
> > Clean up the code in hash.c to use the normal char type for all
> > high-level code, only casting to uint8_t when calling into low-level
> > code.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> The problem is deeper than just this, but we gotta start somewhere...

Is it that much bigger?

Beѕides the usual problem of casts hiding bugs I think we are fine,
but please double check:

 - the lowlevel xfs directory entry hashing code assumes unsigned
   chars, because of that we long compiled with -funsigned-char just
   for XFS, which got obsoleted by the kernel doing it entirely
   after we've switched all the low-level code to use unsigned
   char.
 - given that traditional unix pathnames are just NULL terminate
   by arrays and the 7-bit CI code doesn't even look at the
   high bit we really don't care about signed vs unsigned except
   for the usual C pitfall when casting or shiftting

So as long as all the low-level code uses unsigned char we should
be fine.

Re: [PATCH] db: fix unsigned char related warnings

[Darrick J. Wong]

On Mon, Nov 06, 2023 at 07:59:39AM +0100, Christoph Hellwig wrote:
> On Fri, Nov 03, 2023 at 01:38:13PM -0700, Darrick J. Wong wrote:
> > On Fri, Nov 03, 2023 at 05:02:10PM +0100, Christoph Hellwig wrote:
> > > Clean up the code in hash.c to use the normal char type for all
> > > high-level code, only casting to uint8_t when calling into low-level
> > > code.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > 
> > The problem is deeper than just this, but we gotta start somewhere...
> 
> Is it that much bigger?
> 
> Beѕides the usual problem of casts hiding bugs I think we are fine,
> but please double check:
> 
>  - the lowlevel xfs directory entry hashing code assumes unsigned
>    chars, because of that we long compiled with -funsigned-char just
>    for XFS, which got obsoleted by the kernel doing it entirely
>    after we've switched all the low-level code to use unsigned
>    char.
>  - given that traditional unix pathnames are just NULL terminate
>    by arrays and the 7-bit CI code doesn't even look at the
>    high bit we really don't care about signed vs unsigned except
>    for the usual C pitfall when casting or shiftting
> 
> So as long as all the low-level code uses unsigned char we should
> be fine.

I'll go take a look, though that might take a day or two.

--D