libfsimage: zfs build fix

Hi!

Attached patch makes libfsimage build again on NetBSD.

uchar_t is not defined because both FSYS_ZFS and FSIMAGE are defined at build time.
Also fix warnings with ctype.

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

diff -r bb09e2424a8a tools/libfsimage/zfs/fsys_zfs.c
@@ -80,8 +80,8 @@ static int zio_read_data(blkptr_t *bp, v
 static int
 zfs_bcmp(const void *s1, const void *s2, size_t n)
 {
-	const uchar_t *ps1 = s1;
-	const uchar_t *ps2 = s2;
+	const uint8_t *ps1 = s1;
+	const uint8_t *ps2 = s2;
 
 	if (s1 != s2 && n != 0) {
 		do {

@@ -802,11 +802,11 @@ dnode_get_path(dnode_phys_t *mdn, char *
 	while (*path == '/')
 		path++;
 
-	while (*path && !isspace(*path)) {
+	while (*path && !isspace((uint8_t)*path)) {
 
 		/* get the next component name */
 		cname = path;
-		while (*path && !isspace(*path) && *path != '/')
+		while (*path && !isspace((uint8_t)*path) && *path != '/')
 			path++;
 		ch = *path;
 		*path = 0;   /* ensure null termination */

@@ -914,23 +914,23 @@ get_objset_mdn(dnode_phys_t *mosmdn, cha
 	}
 
 	/* take out the pool name */
-	while (*fsname && !isspace(*fsname) && *fsname != '/')
+	while (*fsname && !isspace((uint8_t)*fsname) && *fsname != '/')
 		fsname++;
 
-	while (*fsname && !isspace(*fsname)) {
+	while (*fsname && !isspace((uint8_t)*fsname)) {
 		uint64_t childobj;
 
 		while (*fsname == '/')
 			fsname++;
 
 		cname = fsname;
-		while (*fsname && !isspace(*fsname) && *fsname != '/')
+		while (*fsname && !isspace((uint8_t)*fsname) && *fsname != '/')
 			fsname++;
 		ch = *fsname;
 		*fsname = 0;
 
 		snapname = cname;
-		while (*snapname && !isspace(*snapname) && *snapname != '@')
+		while (*snapname && !isspace((uint8_t)*snapname) && *snapname != '@')
 			snapname++;
 		if (*snapname == '@') {
 			issnapshot = 1;

diff -r bb09e2424a8a tools/libfsimage/zfs/zfs_lzjb.c
@@ -34,10 +34,10 @@
 int
 lzjb_decompress(void *s_start, void *d_start, size_t s_len, size_t d_len)
 {
-	uchar_t *src = s_start;
-	uchar_t *dst = d_start;
-	uchar_t *d_end = (uchar_t *)d_start + d_len;
-	uchar_t *cpy, copymap = '\0';
+	uint8_t *src = s_start;
+	uint8_t *dst = d_start;
+	uint8_t *d_end = (uint8_t *)d_start + d_len;
+	uint8_t *cpy, copymap = '\0';
 	int copymask = 1 << (NBBY - 1);
 
 	while (dst < d_end) {

@@ -49,7 +49,7 @@ lzjb_decompress(void *s_start, void *d_s
 			int mlen = (src[0] >> (NBBY - MATCH_BITS)) + MATCH_MIN;
 			int offset = ((src[0] << NBBY) | src[1]) & OFFSET_MASK;
 			src += 2;
-			if ((cpy = dst - offset) < (uchar_t *)d_start)
+			if ((cpy = dst - offset) < (uint8_t *)d_start)
 				return (-1);
 			while (--mlen >= 0 && dst < d_end)
 				*dst++ = *cpy++;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Christoph Egger writes ("[Xen-devel] [PATCH] libfsimage: zfs build fix"):

The ctype problems are more than warnings - they would give undefined behaviour if high-bit-set characters are passed.

Ian.

The fsimage zfs patch was changed around to make it simpler to keep in sync with the grub changes. fsys_zfs.c, etc are exact copies of what's in grub e.g.

    http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/grub/grub-0.97/stage2/fsys_zfs.c

fsi_zfs.* are kludges to get the grub code to fit into libfsimage and is the only code which is different from the grub code.

It would be easier to keep in sync if this would be continued.

for this, I would suggest extending this in fsi_zfs.h

#ifdef __linux__
typedef unsigned char uchar_t;
#endif

I didn't write that code and agree that it's pretty ugly :-). I assume you talking about where the char is sign extended or not? Just curious, what's the case that isspace() gives a wrong answer here?

MRJ

Unfortunately with -Wall the zfs sources spewed a quite unreasonable number of warnings. Enough for me to get annoyed, go clean them all up, and add -Werror.

I don't know how important it is to keep the files identical? It's not like interfaces or anything have been changed: all the modifications are trivial one-liners. Basically, it should still be possible to dump a new version of the zfs code into our tree sometime down the line, and then go xfix up the build warnings again (it didn't take that long). Doesn't that suffice? You could even try getting GRUB guys to accept our build cleanups upstream (fat chance given my previous experiences with them, I'll admit). ;-)

-- Keir

Mark Johnson writes ("Re: [Xen-devel] [PATCH] libfsimage: zfs build fix"):

Yes.

If you have high bit set characters then chars are negative. isspace et al take an int which was converted from an unsigned char, and it is not permitted to pass negative numbers to them (except EOF).

So
char *some_string;
...
if (isspace(some_string[14])) {...
is wrong because if some_string[14] is a character >127 (say, 169 which is ISO-LATIN-15 for COPYRIGHT SIGH), then isspace is passed a negative integer (-87).

If you pass a negative number (other than EOF) to isspace, your program may crash or access random data.

Therefore EVERY call to isspace needs to be written like this
if (isspace((unsigned char)some_string[14])) {...

This is of course a design error in the C library.

Ian.