| Message ID | 20231103160210.548636-1-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | db: fix unsigned char related warnings | expand |
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 >
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.
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
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);
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(-)