Skip to contentSkip to conversation

libfsimage: Make it build on NetBSD

PATCH In review Christoph Egger 4 participants 8 messages Sep 21, 2007 – Sep 23, 2007

lore
Share & tools
lore

Christoph Egger

author
#0 lore raw

PATCH 1/1 libfsimage: Make it build on NetBSD

Hi!

Attached patch makes libfsimage build on NetBSD
by fixing a number of those errors:

cc1: warnings being treated as errors fsys_fat.c: In function 'fat_dir':
fsys_fat.c:304: warning: array subscript has type 'char'

Signed-off-by Christoph Egger <[email protected]>
Signature · 12 lines
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy
tools/libfsimage/fat/fsys_fat.c +6 -6
diff -r 40bf3ffff484 tools/libfsimage/fat/fsys_fat.c
@@ -301,7 +301,7 @@ fat_dir (fsi_file_t *ffi, char *dirname)
   /* if we have a real file (and we're not just printing possibilities),
      then this is where we want to exit */
   
-  if (!*dirname || isspace (*dirname))
+  if (!*dirname || isspace ((uint8_t)*dirname))
     {
       if (attrib & FAT_ATTRIB_DIR)
 	{

@@ -325,7 +325,7 @@ fat_dir (fsi_file_t *ffi, char *dirname)
   /* Directories don't have a file size */
   filemax = INT_MAX;
   
-  for (rest = dirname; (ch = *rest) && !isspace (ch) && ch != '/'; rest++);
+  for (rest = dirname; (ch = *rest) && !isspace ((uint8_t)ch) && ch != '/'; rest++);
   
   *rest = 0;
   

@@ -426,13 +426,13 @@ fat_dir (fsi_file_t *ffi, char *dirname)
       {
 	int i, j, c;
 	
-	for (i = 0; i < 8 && (c = filename[i] = tolower (dir_buf[i]))
-	       && !isspace (c); i++);
+	for (i = 0; i < 8 && (c = filename[i] = tolower ((uint8_t)dir_buf[i]))
+	       && !isspace ((uint8_t)c); i++);
 	
 	filename[i++] = '.';
 	
-	for (j = 0; j < 3 && (c = filename[i + j] = tolower (dir_buf[8 + j]))
-	       && !isspace (c); j++);
+	for (j = 0; j < 3 && (c = filename[i + j] = tolower ((uint8_t)dir_buf[8 + j]))
+	       && !isspace ((uint8_t)c); j++);
 	
 	if (j == 0)
 	  i--;
tools/libfsimage/iso9660/fsys_iso9660.c +1 -1
diff -r 40bf3ffff484 tools/libfsimage/iso9660/fsys_iso9660.c
@@ -164,7 +164,7 @@ iso9660_dir (fsi_file_t *ffi, char *dirn
       /* pathlen = strcspn(dirname, "/\n\t "); */
       for (pathlen = 0 ;
 	   dirname[pathlen]
-	     && !isspace(dirname[pathlen]) && dirname[pathlen] != '/' ;
+	     && !isspace((uint8_t)dirname[pathlen]) && dirname[pathlen] != '/' ;
 	   pathlen++)
 	;
tools/libfsimage/reiserfs/fsys_reiserfs.c +4 -4
diff -r 40bf3ffff484 tools/libfsimage/reiserfs/fsys_reiserfs.c
@@ -1029,7 +1029,7 @@ reiserfs_dir (fsi_file_t *ffi, char *dir
 
 	  /* Find out how long our remaining name is. */
 	  len = 0;
-	  while (dirname[len] && !isspace (dirname[len]))
+	  while (dirname[len] && !isspace ((uint8_t)dirname[len]))
 	    len++;
 
 	  if (filemax + len > sizeof (linkbuf) - 1)

@@ -1078,7 +1078,7 @@ reiserfs_dir (fsi_file_t *ffi, char *dir
       /* if we have a real file (and we're not just printing possibilities),
 	 then this is where we want to exit */
       
-      if (! *dirname || isspace (*dirname))
+      if (! *dirname || isspace ((uint8_t)*dirname))
 	{
 	  if (! S_ISREG (mode))
 	    {

@@ -1102,14 +1102,14 @@ reiserfs_dir (fsi_file_t *ffi, char *dir
 	}
       
       /* continue with the file/directory name interpretation */
-      while (*dirname == '/')
+      while ((uint8_t)*dirname == '/')
 	dirname++;
       if (! S_ISDIR (mode))
 	{
 	  errnum = ERR_BAD_FILETYPE;
 	  return 0;
 	}
-      for (rest = dirname; (ch = *rest) && ! isspace (ch) && ch != '/'; rest++);
+      for (rest = dirname; (ch = *rest) && ! isspace ((uint8_t)ch) && ch != '/'; rest++);
       *rest = 0;
       
 # ifndef STAGE1_5
tools/libfsimage/ufs/fsys_ufs.c +2 -2
diff -r 40bf3ffff484 tools/libfsimage/ufs/fsys_ufs.c
@@ -72,13 +72,13 @@ ufs_dir(fsi_file_t *ffi, char *dirname)
 	while (*dirname == '/')
 		dirname++;
 
-	while (inode && *dirname && !isspace(*dirname)) {
+	while (inode && *dirname && !isspace((uint8_t)*dirname)) {
 		if (!openi(ffi, inode))
 			return 0;
 
 		/* parse for next path component */
 		fname = dirname;
-		while (*dirname && !isspace(*dirname) && *dirname != '/')
+		while (*dirname && !isspace((uint8_t)*dirname) && *dirname != '/')
 			dirname++;
 		ch = *dirname;
 		*dirname = 0;	/* ensure null termination */

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

#1 lore raw

tools/libfsimage/fat/fsys_fat.c fat_dir (fsi_file_t *ffi, char *dirname) · line 304

Quoted 2 lines · Christoph Egger
tools/libfsimage/fat/fsys_fat.c +1 -1
-  if (!*dirname || isspace (*dirname))
+  if (!*dirname || isspace ((uint8_t)*dirname))

Eww, surely not. This surely can't give a warning on NetBSD?

john

#2 lore raw
Quoted 4 lines · John Levon

tools/libfsimage/fat/fsys_fat.c fat_dir (fsi_file_t *ffi, char *dirname) · line 304

Quoted 2 lines · Christoph Egger
tools/libfsimage/fat/fsys_fat.c +1 -1
-  if (!*dirname || isspace (*dirname))
+  if (!*dirname || isspace ((uint8_t)*dirname))

Eww, surely not. This surely can't give a warning on NetBSD?

It can if it is implemented as something like:
| int char_isspace[] = {...];
| #define isspace(c) char_isspace[c]

Bastian

Signature · 4 lines
	"Get back to your stations!"
	"We're beaming down to the planet, sir."
		-- Kirk and Mr. Leslie, "This Side of Paradise",
		   stardate 3417.3
#3 lore raw
In reply to Christoph Egger

tools/libfsimage/fat/fsys_fat.c fat_dir (fsi_file_t *ffi, char *dirname) · line 304

Quoted 2 lines · Christoph Egger
tools/libfsimage/fat/fsys_fat.c +1 -1
-  if (!*dirname || isspace (*dirname))
+  if (!*dirname || isspace ((uint8_t)*dirname))

According to C99, the definition is "int isspace(int c)". Even char can be converted implicitely to int. Which problem do you intend to fix with that? If this functions are incorrectly defined on BSD, fix that their.

Bastian

Signature · 2 lines
The heart is not a logical organ.
		-- Dr. Janet Wallace, "The Deadly Years", stardate 3479.4
#4 lore raw
Quoted 7 lines · Bastian Blank

tools/libfsimage/fat/fsys_fat.c fat_dir (fsi_file_t *ffi, char *dirname) · line 304

Quoted 2 lines · Christoph Egger
tools/libfsimage/fat/fsys_fat.c +1 -1
-  if (!*dirname || isspace (*dirname))
+  if (!*dirname || isspace ((uint8_t)*dirname))

According to C99, the definition is "int isspace(int c)". Even char can be converted implicitely to int. Which problem do you intend to fix with that? If this functions are incorrectly defined on BSD, fix that their.

The ISO spec is pretty unambiguous: "The c argument is an int, the value of which the application shall ensure is a character representable as an unsigned char or equal to the value of the macro EOF. If the argument has any other value, the behavior is undefined."

If we sensibly take 'representable' to mean 'representable with the same integer value', then the casts are clearly needed for compliance. This interpretation even makes some sense, since it explicitly disallows collision between character code 0xff and EOF (usually -1).

And read glibc's ctype.h -- it goes to some effort to support 'signed char' input, but makes it clear this is "for broken old programs".

This seems to be one of the ugly corners of the C spec where the truth is not very palatable. :-)

-- Keir

#5 lore raw
Quoted 4 lines · Keir Fraser

The ISO spec is pretty unambiguous: "The c argument is an int, the value of which the application shall ensure is a character representable as an unsigned char or equal to the value of the macro EOF. If the argument has any other value, the behavior is undefined."

§7.4

Quoted 2 lines

If we sensibly take 'representable' to mean 'representable with the same integer value', then the casts are clearly needed for compliance.

No. 'representable' means that the value fits in unsigned char (0-255 on common systems). See example 6 of §5.1.2.3 how the standard uses representable. A cast would force that, if you use unsigned char.

Quoted 2 lines

And read glibc's ctype.h -- it goes to some effort to support 'signed char' input, but makes it clear this is "for broken old programs".

Yeah, it explicitely does the change which the explicite conversion to unsigned char would do.

Quoted 2 lines

This seems to be one of the ugly corners of the C spec where the truth is not very palatable. :-)

Haha. You want some of i + i++?

Bastian

Signature · 2 lines
Virtue is a relative term.
		-- Spock, "Friday's Child", stardate 3499.1
#6 lore raw
Quoted 6 lines · Bastian Blank
Quoted 2 lines

If we sensibly take 'representable' to mean 'representable with the same integer value', then the casts are clearly needed for compliance.

No. 'representable' means that the value fits in unsigned char (0-255 on common systems). See example 6 of §5.1.2.3 how the standard uses representable. A cast would force that, if you use unsigned char.

Well, actually that's what I meant but may have worded poorly. Yes, the range of an unsigned char is [0, 255] and hence no negative integer can be represented. So passing a negative char to a ctype function is invalid.

Quoted 5 lines
Quoted 2 lines

And read glibc's ctype.h -- it goes to some effort to support 'signed char' input, but makes it clear this is "for broken old programs".

Yeah, it explicitely does the change which the explicite conversion to unsigned char would do.

Actually it casts to int, at least in my installed glibc. Thus the sign is maintained, and allowance is made by glibc for negative indexing into its ctype array. I interpret this as a kludge to let old programs and lazy programmers do the obvious thing in spite of the spec.

-- Keir

#7 lore raw
In reply to Christoph Egger
Quoted 6 lines · Christoph Egger

Attached patch makes libfsimage build on NetBSD
by fixing a number of those errors:

cc1: warnings being treated as errors fsys_fat.c: In function 'fat_dir':
fsys_fat.c:304: warning: array subscript has type 'char'

You also cast before a comparison with a char literal ('/'). Surely that isn't necessary?

For the rest, it does sound like NetBSD's ctype.h is strictly compliant with ISO C, which requires a non-EOF argument to be representable as an unsigned char. Clearly a negative argument is not representable as an unsigned char of the same integer value, so a cast is needed to force things to comply with the ISO spec. Bah.

-- Keir