Allow build without dirent.d_type, fix "ls (host)" crash

PATCH In review Christian Franke 3 participants 7 messages Oct 23, 2007 – Nov 18, 2007

Share & tools

Christian Franke

author

2007-10-23 19:50 UTC #0 lore raw

PATCH 1/1 Allow build without dirent.d_type, fix "ls (host)" crash

This patch enables compilation on systems without the BSD extension dirent.d_type. It relies on #define DT_DIR, won't work for enums (glibc dirent.h contains both :-).

An alternative would be to include is_dir() unconditionally and call it also when d_type == DT_UNKNOWN.

The patch also fixes a grub-emu crash.

Christian

2007-10-23 Christian Franke <[email protected]>

* util/hostfs.c (is_dir): New function. (grub_hostfs_dir): Handle missing dirent.d_type case. (grub_hostfs_label): Clear label pointer. This fixes a crash of grub-emu on "ls (host)".

util/hostfs.c +28 -0

@@ -25,6 +25,29 @@
 #include <dirent.h>
 #include <stdio.h>
 
+
+#ifndef DT_DIR
+/* dirent.d_type is a BSD extension, not part of POSIX */
+#include <sys/stat.h>
+#include <string.h>
+
+static int
+is_dir(const char *path, const char *name)
+{
+  int len1 = strlen(path), len2 = strlen(name);
+  char pathname[len1+1+len2+1+13];
+  struct stat st;
+  strcpy (pathname, path);
+  /* Avoid UNC-path "//name" on Cygwin */
+  if (len1 > 0 && pathname[len1-1] != '/')
+    strcat (pathname, "/");
+  strcat (pathname, name);
+  if (stat (pathname, &st))
+    return 0;
+  return S_ISDIR(st.st_mode);
+}
+#endif
+
 static grub_err_t
 grub_hostfs_dir (grub_device_t device, const char *path, 
 		 int (*hook) (const char *filename, int dir))

@@ -48,7 +71,11 @@ grub_hostfs_dir (grub_device_t device, c
       if (! de)
 	break;
 
+#ifdef DT_DIR
       hook (de->d_name, de->d_type == DT_DIR);
+#else
+      hook (de->d_name, is_dir(path, de->d_name));
+#endif
     }
 
   closedir (dir);

@@ -101,6 +128,7 @@ static grub_err_t
 grub_hostfs_label (grub_device_t device __attribute ((unused)),
 		   char **label __attribute ((unused)))
 {
+  *label = 0;
   return GRUB_ERR_NONE;
 }

Christian Franke

author

2007-11-02 20:59 UTC #1 lore raw

Second version of the patch.

It fixes another bug: missing fseek() in grub_hostfs_read().

Christian

2007-11-02 Christian Franke <[email protected]>

* util/hostfs.c (is_dir): New function. (grub_hostfs_dir): Handle missing dirent.d_type case. (grub_hostfs_read): Add missing fseek(). (grub_hostfs_label): Clear label pointer. This fixes a crash of grub-emu on "ls (host)".

util/hostfs.c +29 -0

@@ -25,6 +25,29 @@
 #include <dirent.h>
 #include <stdio.h>
 
+
+#ifndef DT_DIR
+/* dirent.d_type is a BSD extension, not part of POSIX */
+#include <sys/stat.h>
+#include <string.h>
+
+static int
+is_dir(const char *path, const char *name)
+{
+  int len1 = strlen(path), len2 = strlen(name);
+  char pathname[len1+1+len2+1+13];
+  struct stat st;
+  strcpy (pathname, path);
+  /* Avoid UNC-path "//name" on Cygwin */
+  if (len1 > 0 && pathname[len1-1] != '/')
+    strcat (pathname, "/");
+  strcat (pathname, name);
+  if (stat (pathname, &st))
+    return 0;
+  return S_ISDIR(st.st_mode);
+}
+#endif
+
 static grub_err_t
 grub_hostfs_dir (grub_device_t device, const char *path, 
 		 int (*hook) (const char *filename, int dir))

@@ -48,7 +71,11 @@ grub_hostfs_dir (grub_device_t device, c
       if (! de)
 	break;
 
+#ifdef DT_DIR
       hook (de->d_name, de->d_type == DT_DIR);
+#else
+      hook (de->d_name, is_dir(path, de->d_name));
+#endif
     }
 
   closedir (dir);

@@ -81,6 +108,7 @@ grub_hostfs_read (grub_file_t file, char
   FILE *f;
 
   f = (FILE *) file->data;
+  fseek (f, file->offset, SEEK_SET);
   int s= fread (buf, 1, len, f);
 
   return s;

@@ -101,6 +129,7 @@ static grub_err_t
 grub_hostfs_label (grub_device_t device __attribute ((unused)),
 		   char **label __attribute ((unused)))
 {
+  *label = 0;
   return GRUB_ERR_NONE;
 }

Marco Gerards

2007-11-09 14:54 UTC #2 lore raw

Quoted 20 lines · Christian Franke

Second version of the patch.
It fixes another bug: missing fseek() in grub_hostfs_read().
Christian
2007-11-02 Christian Franke <[email protected]>
* util/hostfs.c (is_dir): New function. (grub_hostfs_dir): Handle missing dirent.d_type case. (grub_hostfs_read): Add missing fseek(). (grub_hostfs_label): Clear label pointer. This fixes a crash of grub-emu on "ls (host)".
util/hostfs.c +0 -0
@@ -25,6 +25,29 @@
 #include <dirent.h>

util/hostfs.c line 35

Quoted 10 lines

util/hostfs.c +8 -0

 #include <stdio.h>
 
+
+#ifndef DT_DIR
+/* dirent.d_type is a BSD extension, not part of POSIX */
+#include <sys/stat.h>
+#include <string.h>
+
+static int
+is_dir(const char *path, const char *name)

is_dir (

util/hostfs.c line 37

Quoted 2 lines

util/hostfs.c +2 -0

+{
+  int len1 = strlen(path), len2 = strlen(name);

Please split this up. Or even better use separate declarations and code.

util/hostfs.c line 38

Quoted 1 line

util/hostfs.c +1 -0

+  char pathname[len1+1+len2+1+13];

Please add spaces around binary operators.

util/hostfs.c line 40

Quoted 2 lines

util/hostfs.c +2 -0

+  struct stat st;
+  strcpy (pathname, path);

Please add more blank lines between your code to make it easier to read. :-)

util/hostfs.c line 41

Quoted 1 line

util/hostfs.c +1 -0

+  /* Avoid UNC-path "//name" on Cygwin */

". " like explained in a previous email.

util/hostfs.c line 49

Quoted 8 lines

util/hostfs.c +8 -0

+  if (len1 > 0 && pathname[len1-1] != '/')
+    strcat (pathname, "/");
+  strcat (pathname, name);
+  if (stat (pathname, &st))
+    return 0;
+  return S_ISDIR(st.st_mode);
+}
+#endif

Why can't you just use S_ISDIR?

Quoted 16 lines

util/hostfs.c +4 -0

 static grub_err_t
 grub_hostfs_dir (grub_device_t device, const char *path, 
 		 int (*hook) (const char *filename, int dir))

@@ -48,7 +71,11 @@ grub_hostfs_dir (grub_device_t device, c
       if (! de)
 	break;
 
+#ifdef DT_DIR
       hook (de->d_name, de->d_type == DT_DIR);
+#else
+      hook (de->d_name, is_dir(path, de->d_name));
+#endif
     }
 
   closedir (dir);

util/hostfs.c grub_hostfs_read (grub_file_t file, char · line 112

Quoted 5 lines

util/hostfs.c +1 -0

@@ -81,6 +108,7 @@ grub_hostfs_read (grub_file_t file, char
   FILE *f;
 
   f = (FILE *) file->data;
+  fseek (f, file->offset, SEEK_SET);
   int s= fread (buf, 1, len, f);

"int s = "

Quoted 13 lines

util/hostfs.c +1 -0
   return s;
@@ -101,6 +129,7 @@ static grub_err_t
 grub_hostfs_label (grub_device_t device __attribute ((unused)),
 		   char **label __attribute ((unused)))
 {
+  *label = 0;
   return GRUB_ERR_NONE;
 }
 
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Christian Franke

author

2007-11-09 21:07 UTC #3 lore raw

Quoted 8 lines · Marco Gerards

Review
Quoted 4 lines
...
(quoted diff) +2 -0
+static int
+is_dir(const char *path, const char *name)
is_dir (

OK. Too many projects, too many policies, sorry :-)

Quoted 8 lines

Review
Quoted 3 lines
(quoted diff) +2 -0
+{
+  int len1 = strlen(path), len2 = strlen(name);
Please split this up. Or even better use separate declarations and code.

Yes. No. See comment below.

Quoted 6 lines

Quoted 2 lines
+ char pathname[len1+1+len2+1+13];
Please add spaces around binary operators.

Quoted 8 lines

Review
Quoted 3 lines
(quoted diff) +2 -0
+  struct stat st;
+  strcpy (pathname, path);
Please add more blank lines between your code to make it easier to read. :-)

OK.

Quoted 13 lines

Review
Quoted 9 lines
...
(quoted diff) +7 -0
+    strcat (pathname, "/");
+  strcat (pathname, name);
+  if (stat (pathname, &st))
+    return 0;
+  return S_ISDIR(st.st_mode);
+}
+#endif
Why can't you just use S_ISDIR?

Quoted 4 lines

...

(quoted diff) +0 -0

@@ -81,6 +108,7 @@ grub_hostfs_read (grub_file_t file, char
   FILE *f;

Quoted 8 lines

Review grub_hostfs_read (grub_file_t file, char · line 113
Quoted 4 lines
(quoted diff) +1 -0
   f = (FILE *) file->data;
+  fseek (f, file->offset, SEEK_SET);
   int s= fread (buf, 1, len, f);
 
"int s = "

Code janitor work outside the scope of this patch ... done ;-)

BTW: This line uses "declaration statement" syntax, therefore this is apparently accepted in GRUB2 codebase :-) I definitely prefer this (first use = declaration = initialization) style, which is state of the art in most (all?) modern languages. This C++ (1986) feature and early GCC (1.*) extension was finally included into C99, so it is portable now. is_dir() rewritten accordingly.

Christian

2007-11-09 Christian Franke <[email protected]>

util/hostfs.c +35 -1

@@ -25,6 +25,34 @@
 #include <dirent.h>
 #include <stdio.h>
 
+
+#ifndef DT_DIR
+/* dirent.d_type is a BSD extension, not part of POSIX */
+#include <sys/stat.h>
+#include <string.h>
+
+static int
+is_dir (const char *path, const char *name)
+{
+  int len1 = strlen(path);
+  int len2 = strlen(name);
+
+  char pathname[len1 + 1 + len2 + 1 + 13];
+  strcpy (pathname, path);
+
+  /* Avoid UNC-path "//name" on Cygwin.  */
+  if (len1 > 0 && pathname[len1 - 1] != '/')
+    strcat (pathname, "/");
+
+  strcat (pathname, name);
+
+  struct stat st;
+  if (stat (pathname, &st))
+    return 0;
+  return S_ISDIR (st.st_mode);
+}
+#endif
+
 static grub_err_t
 grub_hostfs_dir (grub_device_t device, const char *path, 
 		 int (*hook) (const char *filename, int dir))

@@ -48,7 +76,11 @@
       if (! de)
 	break;
 
+#ifdef DT_DIR
       hook (de->d_name, de->d_type == DT_DIR);
+#else
+      hook (de->d_name, is_dir (path, de->d_name));
+#endif
     }
 
   closedir (dir);

@@ -81,7 +113,8 @@
   FILE *f;
 
   f = (FILE *) file->data;
-  int s= fread (buf, 1, len, f);
+  fseek (f, file->offset, SEEK_SET);
+  int s = fread (buf, 1, len, f);
 
   return s;
 }

@@ -101,6 +134,7 @@
 grub_hostfs_label (grub_device_t device __attribute ((unused)),
 		   char **label __attribute ((unused)))
 {
+  *label = 0;
   return GRUB_ERR_NONE;
 }

Marco Gerards

2007-11-10 16:03 UTC #4 lore raw

Quoted 11 lines · Christian Franke

Quoted 8 lines · Marco Gerards
Review
Quoted 4 lines
...
(quoted diff) +2 -0
+static int
+is_dir(const char *path, const char *name)
is_dir (

Quoted 1 line

OK. Too many projects, too many policies, sorry :-)

np, don't worry :-)

Hopefully you are not annoyed by my comments. A consistent style throughout GRUB is important to me.

Quoted 43 lines

Quoted 8 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
+{
+  int len1 = strlen(path), len2 = strlen(name);
Please split this up. Or even better use separate declarations and code.
Yes. No. See comment below.
Quoted 6 lines
Quoted 2 lines
+ char pathname[len1+1+len2+1+13];
Please add spaces around binary operators.
Quoted 8 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
+  struct stat st;
+  strcpy (pathname, path);
Please add more blank lines between your code to make it easier to read. :-)
OK.
Quoted 13 lines
util/hostfs.c line 54
Quoted 9 lines
...
util/hostfs.c +7 -0
+    strcat (pathname, "/");
+  strcat (pathname, name);
+  if (stat (pathname, &st))
+    return 0;
+  return S_ISDIR(st.st_mode);
+}
+#endif
Why can't you just use S_ISDIR?

Quoted 1 line

??

Nevermind, I wasn't really awake ;)

Quoted 12 lines

Quoted 11 lines
util/hostfs.c grub_hostfs_read (grub_file_t file, char · line 112
Quoted 7 lines
...
util/hostfs.c +1 -0
@@ -81,6 +108,7 @@ grub_hostfs_read (grub_file_t file, char
   FILE *f;
    f = (FILE *) file->data;
+  fseek (f, file->offset, SEEK_SET);
   int s= fread (buf, 1, len, f);
 
"int s = "

Quoted 1 line

Code janitor work outside the scope of this patch ... done ;-)

LOL! Sorry, I was a bit enthousiastic ;-)

Thanks! :)

Quoted 7 lines

BTW: This line uses "declaration statement" syntax, therefore this is apparently accepted in GRUB2 codebase :-)
I definitely prefer this (first use = declaration = initialization) style, which is state of the art in most (all?) modern languages. This C++ (1986) feature and early GCC (1.*) extension was finally included into C99, so it is portable now.
is_dir() rewritten accordingly.

It is. The problem is that you had two function calls on one line. I want to avoid that.

Quoted 9 lines

Christian
2007-11-09 Christian Franke <[email protected]>
* util/hostfs.c (is_dir): New function. (grub_hostfs_dir): Handle missing dirent.d_type case. (grub_hostfs_read): Add missing fseek(). (grub_hostfs_label): Clear label pointer. This fixes a crash of grub-emu on "ls (host)".

Looks fine to me.

btw, when do you use (host)? It's intended as a debugging tool.

--
Marco

Christian Franke

author

2007-11-10 18:16 UTC #5 lore raw

Quoted 12 lines · Marco Gerards

Quoted 9 lines
...
2007-11-09 Christian Franke <[email protected]>
* util/hostfs.c (is_dir): New function. (grub_hostfs_dir): Handle missing dirent.d_type case. (grub_hostfs_read): Add missing fseek(). (grub_hostfs_label): Clear label pointer. This fixes a crash of grub-emu on "ls (host)".
Looks fine to me.

Thanks.

Quoted 2 lines

btw, when do you use (host)? It's intended as a debugging tool.

It was there, it didn't work, and it breaked remaining grub-emu device access at all. So I fixed it :-)

Christian

Robert Millan

2007-11-18 07:14 UTC #6 lore raw

In reply to Marco Gerards

Quoted 10 lines · Marco Gerards

Quoted 8 lines
2007-11-09 Christian Franke <[email protected]>
* util/hostfs.c (is_dir): New function. (grub_hostfs_dir): Handle missing dirent.d_type case. (grub_hostfs_read): Add missing fseek(). (grub_hostfs_label): Clear label pointer. This fixes a crash of grub-emu on "ls (host)".
Looks fine to me.

Committed.

Signature · 5 lines

Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)