[BUGFIX] Don't use DT_DIR: It doesn't work on non-ext* filesystems

[BUGFIX] Don't use DT_DIR: It doesn't work on non-ext* filesystems

[Vladimir 'phcoder' Serbinenko]

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

-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

[-- Attachment #2: dir.diff --]
[-- Type: text/plain, Size: 1082 bytes --]

diff --git a/ChangeLog b/ChangeLog
index a0780ab..16aab92 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2009-07-23  Vladimir Serbinenko  <phcoder@gmail.com>
+
+	* util/hostfs.c (grub_hostfs_dir): Don't use DT_DIR: It doesn't work
+	on XFS or ReiserFS.
+
 2009-07-21  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	UUID support for UFS
diff --git a/util/hostfs.c b/util/hostfs.c
index 1b963bb..83db2f1 100644
--- a/util/hostfs.c
+++ b/util/hostfs.c
@@ -28,7 +28,6 @@
 #include <stdio.h>
 
 
-#ifndef DT_DIR
 /* dirent.d_type is a BSD extension, not part of POSIX */
 #include <sys/stat.h>
 #include <string.h>
@@ -53,7 +52,6 @@ is_dir (const char *path, const char *name)
     return 0;
   return S_ISDIR (st.st_mode);
 }
-#endif
 
 static grub_err_t
 grub_hostfs_dir (grub_device_t device, const char *path,
@@ -81,11 +79,7 @@ grub_hostfs_dir (grub_device_t device, const char *path,
       if (! de)
 	break;
 
-#ifdef DT_DIR
-      info.dir = (de->d_type == DT_DIR);
-#else
       info.dir = !! is_dir (path, de->d_name);
-#endif
       hook (de->d_name, &info);
 
     }

Re: [BUGFIX] Don't use DT_DIR: It doesn't work on non-ext* filesystems

[Pavel Roskin]

On Thu, 2009-07-23 at 11:29 +0200, Vladimir 'phcoder' Serbinenko wrote:

> -#ifdef DT_DIR
> -      info.dir = (de->d_type == DT_DIR);
> -#else
>        info.dir = !! is_dir (path, de->d_name);
> -#endif

Fine with me.  Finally a patch that reduces the number of preprocessor
directives :-)

-- 
Regards,
Pavel Roskin

Re: [BUGFIX] Don't use DT_DIR: It doesn't work on non-ext* filesystems

[Christian Franke]

Pavel Roskin wrote:
> On Thu, 2009-07-23 at 11:29 +0200, Vladimir 'phcoder' Serbinenko wrote:
>
>   
>> -#ifdef DT_DIR
>> -      info.dir = (de->d_type == DT_DIR);
>> -#else
>>        info.dir = !! is_dir (path, de->d_name);
>> -#endif
>>     
>
> Fine with me.  Finally a patch that reduces the number of preprocessor
> directives :-)
>
>   

:-)

Grub hostfs has no performance requirement like e.g. find. The 
optimization with d_type can simply be ignored - patch is OK.

The idea behind d_type is that it should only be set != DT_UNKNOWN if 
the type info is available for free or at low cost. It is safe for a 
filesystem implementation to return DT_UNKNOWN always.

A correct performance-aware solution would look like:

#ifdef DT_DIR
  if (de->d_type == DT_DIR)
    info.dir = 1;
  else if (de->type == DT_FILE)
    info.dir = 0;
  else
#endif
    info.dir = !! is_dir (path, de->d_name);

This was actually wrong in my original patch (svn 1353) that was 
necessary because of missing d_type on Cygwin - Sorry! Meantime I added 
d_type support to Cygwin itself and therefore learned how this is 
supposed to work :-)

-- 
Regards,
Christian Franke

Re: [BUGFIX] Don't use DT_DIR: It doesn't work on non-ext* filesystems

[Pavel Roskin]

On Fri, 2009-07-24 at 23:02 +0200, Christian Franke wrote:

> A correct performance-aware solution would look like:
> 
> #ifdef DT_DIR
>   if (de->d_type == DT_DIR)
>     info.dir = 1;
>   else if (de->type == DT_FILE)

There in no DT_FILE in glibc, but there is DT_REG.  DT_UNKNOWN is
present.  Perhaps the above line should be

else if (de->type != DT_UNKNOWN)

We only care if it's a directory or not.  All other objects can be
treated like files.

I'm fine either way, whether we fix the "high-performance" code or
remove it, as long as we don't have to add more checks.

-- 
Regards,
Pavel Roskin

Re: [BUGFIX] Don't use DT_DIR: It doesn't work on non-ext* filesystems

[Christian Franke]

Pavel Roskin wrote:
> On Fri, 2009-07-24 at 23:02 +0200, Christian Franke wrote:
>
>   
>> A correct performance-aware solution would look like:
>>
>> #ifdef DT_DIR
>>   if (de->d_type == DT_DIR)
>>     info.dir = 1;
>>   else if (de->type == DT_FILE)
>>     
>
> There in no DT_FILE in glibc, but there is DT_REG.  

Yes correct.


> DT_UNKNOWN is
> present.  Perhaps the above line should be
>
> else if (de->type != DT_UNKNOWN)
>
> We only care if it's a directory or not.  All other objects can be
> treated like files.
>
>   

It depends: If DT_UNKNOWN is used, you have to replace stat() by lstat() 
in is_dir(). Otherwise symlinked directories would be appear as dir only 
if d_type == DT_UNKNOWN and not if d_type == DT_LNK.


> I'm fine either way, whether we fix the "high-performance" code or
> remove it, as long as we don't have to add more checks.
>
>   

I would suggest to remove it.

-- 
Regards,
Christian Franke

Re: [BUGFIX] Don't use DT_DIR: It doesn't work on non-ext* filesystems

[Robert Millan]

On Fri, Jul 24, 2009 at 05:49:19PM -0400, Pavel Roskin wrote:
> On Fri, 2009-07-24 at 23:02 +0200, Christian Franke wrote:
> 
> > A correct performance-aware solution would look like:
> > 
> > #ifdef DT_DIR
> >   if (de->d_type == DT_DIR)
> >     info.dir = 1;
> >   else if (de->type == DT_FILE)
> 
> There in no DT_FILE in glibc, but there is DT_REG.  DT_UNKNOWN is
> present.  Perhaps the above line should be
> 
> else if (de->type != DT_UNKNOWN)
> 
> We only care if it's a directory or not.  All other objects can be
> treated like files.
> 
> I'm fine either way, whether we fix the "high-performance" code or
> remove it, as long as we don't have to add more checks.

d_type doesn't always exist.  E.g. on OpenSolaris.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."