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

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

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

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  <franke@computer.org>

	* 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)".




[-- Attachment #2: grub2-hostfs-d_type.patch --]
[-- Type: text/x-patch, Size: 1300 bytes --]

--- grub2.orig/util/hostfs.c	2007-08-02 19:24:06.000000000 +0200
+++ grub2/util/hostfs.c	2007-10-13 17:17:20.000000000 +0200
@@ -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;
 }
 

Re: [PATCH] Allow build without dirent.d_type, fix "ls (host)" crash

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 389 bytes --]

Second version of the patch.

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

Christian

2007-11-02  Christian Franke  <franke@computer.org>

	* 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)".




[-- Attachment #2: grub2-hostfs-d_type-2.patch --]
[-- Type: text/x-patch, Size: 1489 bytes --]

--- grub2.orig/util/hostfs.c	2007-08-02 19:24:06.000000000 +0200
+++ grub2/util/hostfs.c	2007-11-02 21:31:37.703125000 +0100
@@ -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;
 }
 

Re: [PATCH] Allow build without dirent.d_type, fix "ls (host)" crash

[Marco Gerards]

Christian Franke <Christian.Franke@t-online.de> writes:

> Second version of the patch.
>
> It fixes another bug: missing fseek() in grub_hostfs_read().
>
> Christian
>
> 2007-11-02  Christian Franke  <franke@computer.org>
>
> 	* 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)".
>
>
>
> --- grub2.orig/util/hostfs.c	2007-08-02 19:24:06.000000000 +0200
> +++ grub2/util/hostfs.c	2007-11-02 21:31:37.703125000 +0100
> @@ -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)

is_dir (

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

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

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

Please add spaces around binary operators.

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

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

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

".  " like explained in a previous email.

> +  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?

>  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);

"int s = "

>    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
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Allow build without dirent.d_type, fix "ls (host)" crash

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 1827 bytes --]

Marco Gerards wrote:

>> ...
>> +static int
>> +is_dir(const char *path, const char *name)
>>     
>
> is_dir (
>
>   

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


>> +{
>> +  int len1 = strlen(path), len2 = strlen(name);
>>     
>
> Please split this up.  Or even better use separate declarations and
> code.
>
>   

Yes. No. See comment below.


>> +  char pathname[len1+1+len2+1+13];
>>     
>
> Please add spaces around binary operators.
>
>   
>> +  struct stat st;
>> +  strcpy (pathname, path);
>>     
>
> Please add more blank lines between your code to make it easier to
> read. :-)
>
>   

OK.


>> ...
>> +    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?
>
>   

??


>> ...
>> @@ -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 = "
>
>   

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  <franke@computer.org>

	* 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)".




[-- Attachment #2: grub2-hostfs-d_type-3.patch --]
[-- Type: text/x-patch, Size: 1457 bytes --]

--- grub2.orig/util/hostfs.c	2007-08-02 19:24:06.000000000 +0200
+++ grub2/util/hostfs.c	2007-11-09 21:41:42.484375000 +0100
@@ -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;
 }
 

Re: [PATCH] Allow build without dirent.d_type, fix "ls (host)" crash

[Marco Gerards]

Christian Franke <Christian.Franke@t-online.de> writes:

> Marco Gerards wrote:
>
>>> ...
>>> +static int
>>> +is_dir(const char *path, const char *name)
>>>
>>
>> is_dir (
>>
>>
>
> 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.

>>> +{
>>> +  int len1 = strlen(path), len2 = strlen(name);
>>>
>>
>> Please split this up.  Or even better use separate declarations and
>> code.
>>
>>
>
> Yes. No. See comment below.
>
>>> +  char pathname[len1+1+len2+1+13];
>>>
>>
>> Please add spaces around binary operators.
>>
>>
>>> +  struct stat st;
>>> +  strcpy (pathname, path);
>>>
>>
>> Please add more blank lines between your code to make it easier to
>> read. :-)
>>
>>
>
> OK.
>
>
>>> ...
>>> +    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?
>>
>>
>
> ??

Nevermind, I wasn't really awake ;)

>>> ...
>>> @@ -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 = "
>>
>>
>
> Code janitor work outside the scope of this patch ... done ;-)

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

Thanks! :)

> 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.

> Christian
>
> 2007-11-09  Christian Franke  <franke@computer.org>
>
> 	* 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

Re: [PATCH] Allow build without dirent.d_type, fix "ls (host)" crash

[Christian Franke]

Marco Gerards wrote:
>> ...
>> 2007-11-09  Christian Franke  <franke@computer.org>
>>
>> 	* 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.

> 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

Re: [PATCH] Allow build without dirent.d_type, fix "ls (host)" crash

[Robert Millan]

On Sat, Nov 10, 2007 at 05:03:48PM +0100, Marco Gerards wrote:
> >
> > 2007-11-09  Christian Franke  <franke@computer.org>
> >
> > 	* 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.

-- 
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 /.)