[Patch] Add dirent.d_type support to Cygwin 1.7 ?

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[Patch] Add dirent.d_type support to Cygwin 1.7 ?

Christian Franke
This is an experimental patch to add dirent.d_type support to readdir().
It sets d_type to DT_DIR/REG for normal disk directories/files and
DT_UNKNOWN in all other cases.

Test result with original find (4.4.0-3) vs. same find rebuild with new
sys/dirent.h:

$ export TIMEFORMAT='%1R'

$ time find /cygdrive/c/cygwin >/dev/null
30.5

$ time find-with-d_type /cygdrive/c/cygwin >/dev/null
9.5

$ time cmd /c dir /a/s 'c:\cygwin' >/dev/null
5.2

Due to the missing initialization of '__d_unused1', new programs with
d_type support would not be backward compatible.


Christian


diff --git a/winsup/cygwin/dir.cc b/winsup/cygwin/dir.cc
index 30662e6..c200469 100644
--- a/winsup/cygwin/dir.cc
+++ b/winsup/cygwin/dir.cc
@@ -93,6 +93,11 @@ readdir_worker (DIR *dir, dirent *de)
     }
 
   de->d_ino = 0;
+#ifdef _DIRENT_HAVE_D_TYPE
+  de->d_type = DT_UNKNOWN;
+#endif
+  memset (&de->__d_unused1, 0, sizeof (de->__d_unused1));
+
   int res = ((fhandler_base *) dir->__fh)->readdir (dir, de);
 
   if (res == ENMFILE)
diff --git a/winsup/cygwin/fhandler_disk_file.cc b/winsup/cygwin/fhandler_disk_file.cc
index e0880f0..c748e24 100644
--- a/winsup/cygwin/fhandler_disk_file.cc
+++ b/winsup/cygwin/fhandler_disk_file.cc
@@ -1677,6 +1677,28 @@ fhandler_disk_file::readdir_helper (DIR *dir, dirent *de, DWORD w32_err,
       dir->__flags &= ~dirent_set_d_ino;
     }
 
+#ifdef _DIRENT_HAVE_D_TYPE
+  /* Set d_type if type can be determined from file attributes.
+     FILE_ATTRIBUTE_SYSTEM ommitted to leave DT_UNKNOWN for old symlinks.
+     For new symlinks, d_type will be reset to DT_UNKNOWN below.  */
+  if (attr &&
+      !(attr & ~( FILE_ATTRIBUTE_NORMAL
+                | FILE_ATTRIBUTE_READONLY
+                | FILE_ATTRIBUTE_ARCHIVE
+                | FILE_ATTRIBUTE_HIDDEN
+                | FILE_ATTRIBUTE_COMPRESSED
+                | FILE_ATTRIBUTE_ENCRYPTED
+                | FILE_ATTRIBUTE_SPARSE_FILE
+                | FILE_ATTRIBUTE_NOT_CONTENT_INDEXED
+                | FILE_ATTRIBUTE_DIRECTORY)))
+    {
+      if (attr & FILE_ATTRIBUTE_DIRECTORY)
+        de->d_type = DT_DIR;
+      else
+        de->d_type = DT_REG;
+    }
+#endif
+
   /* Check for directory reparse point.  These are potential volume mount
      points which have another inode than the underlying directory. */
   if ((attr & (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_REPARSE_POINT))
@@ -1728,7 +1750,12 @@ fhandler_disk_file::readdir_helper (DIR *dir, dirent *de, DWORD w32_err,
     }
   path_conv fpath (&fbuf, PC_SYM_NOFOLLOW);
   if (fpath.issymlink () || fpath.is_fs_special ())
-    fname->Length -= 4 * sizeof (WCHAR);
+            {
+      fname->Length -= 4 * sizeof (WCHAR);
+#ifdef _DIRENT_HAVE_D_TYPE
+              de->d_type = DT_UNKNOWN;
+#endif
+            }
  }
     }
 
diff --git a/winsup/cygwin/include/sys/dirent.h b/winsup/cygwin/include/sys/dirent.h
index 41bfcc1..d782e58 100644
--- a/winsup/cygwin/include/sys/dirent.h
+++ b/winsup/cygwin/include/sys/dirent.h
@@ -18,11 +18,17 @@
 
 #pragma pack(push,4)
 #if defined(__INSIDE_CYGWIN__) || defined (__CYGWIN_USE_BIG_TYPES__)
+#define _DIRENT_HAVE_D_TYPE
 struct dirent
 {
   long __d_version; /* Used internally */
   __ino64_t d_ino;
+#ifdef _DIRENT_HAVE_D_TYPE
+  unsigned char d_type;
+  unsigned char __d_unused1[3];
+#else
   __uint32_t __d_unused1;
+#endif
   __uint32_t __d_internal1;
   char d_name[NAME_MAX + 1];
 };
@@ -36,6 +42,8 @@ struct dirent
   char d_name[NAME_MAX + 1];
 };
 #endif
+/* Compile time size check.  */
+typedef char __ASSERT_SIZEOF_dirent[sizeof(struct dirent) == 20+NAME_MAX+1 ? 1 : -1];
 #pragma pack(pop)
 
 #define __DIRENT_COOKIE 0xdede4242
@@ -77,7 +85,7 @@ int scandir (const char *__dir,
      int (*compar) (const struct dirent **, const struct dirent **));
 
 int alphasort (const struct dirent **__a, const struct dirent **__b);
-#if 0  /* these make no sense in the absence of d_type */
+#ifdef _DIRENT_HAVE_D_TYPE
 /* File types for `d_type'.  */
 enum
 {
@@ -104,6 +112,6 @@ enum
 /* Convert between stat structure types and directory types.  */
 # define IFTODT(mode) (((mode) & 0170000) >> 12)
 # define DTTOIF(dirtype)        ((dirtype) << 12)
-#endif /* #if 0 */
+#endif /* _DIRENT_HAVE_D_TYPE */
 #endif /* _POSIX_SOURCE */
 #endif /*_SYS_DIRENT_H*/
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Add dirent.d_type support to Cygwin 1.7 ?

Christopher Faylor-8
On Wed, Nov 26, 2008 at 10:24:14PM +0100, Christian Franke wrote:

> This is an experimental patch to add dirent.d_type support to readdir(). It
> sets d_type to DT_DIR/REG for normal disk directories/files and DT_UNKNOWN
> in all other cases.
>
> Test result with original find (4.4.0-3) vs. same find rebuild with new
> sys/dirent.h:
>
> $ export TIMEFORMAT='%1R'
>
> $ time find /cygdrive/c/cygwin >/dev/null
> 30.5
>
> $ time find-with-d_type /cygdrive/c/cygwin >/dev/null
> 9.5
>
> $ time cmd /c dir /a/s 'c:\cygwin' >/dev/null
> 5.2
>
> Due to the missing initialization of '__d_unused1', new programs with
> d_type support would not be backward compatible.
>
>
> Christian
>

>diff --git a/winsup/cygwin/dir.cc b/winsup/cygwin/dir.cc
>index 30662e6..c200469 100644
>--- a/winsup/cygwin/dir.cc
>+++ b/winsup/cygwin/dir.cc
>@@ -93,6 +93,11 @@ readdir_worker (DIR *dir, dirent *de)
>     }
>
>   de->d_ino = 0;
>+#ifdef _DIRENT_HAVE_D_TYPE
>+  de->d_type = DT_UNKNOWN;
>+#endif
>+  memset (&de->__d_unused1, 0, sizeof (de->__d_unused1));
>+

I don't see a need for a conditional here.  If this is added Cygwin
supports d_type.

>   int res = ((fhandler_base *) dir->__fh)->readdir (dir, de);
>
>   if (res == ENMFILE)
>diff --git a/winsup/cygwin/fhandler_disk_file.cc b/winsup/cygwin/fhandler_disk_file.cc
>index e0880f0..c748e24 100644
>--- a/winsup/cygwin/fhandler_disk_file.cc
>+++ b/winsup/cygwin/fhandler_disk_file.cc
>@@ -1677,6 +1677,28 @@ fhandler_disk_file::readdir_helper (DIR *dir, dirent *de, DWORD w32_err,
>       dir->__flags &= ~dirent_set_d_ino;
>     }
>
>+#ifdef _DIRENT_HAVE_D_TYPE
>+  /* Set d_type if type can be determined from file attributes.
>+     FILE_ATTRIBUTE_SYSTEM ommitted to leave DT_UNKNOWN for old symlinks.
>+     For new symlinks, d_type will be reset to DT_UNKNOWN below.  */
>+  if (attr &&
>+      !(attr & ~( FILE_ATTRIBUTE_NORMAL
>+                | FILE_ATTRIBUTE_READONLY
>+                | FILE_ATTRIBUTE_ARCHIVE
>+                | FILE_ATTRIBUTE_HIDDEN
>+                | FILE_ATTRIBUTE_COMPRESSED
>+                | FILE_ATTRIBUTE_ENCRYPTED
>+                | FILE_ATTRIBUTE_SPARSE_FILE
>+                | FILE_ATTRIBUTE_NOT_CONTENT_INDEXED
>+                | FILE_ATTRIBUTE_DIRECTORY)))
>+    {
>+      if (attr & FILE_ATTRIBUTE_DIRECTORY)
>+        de->d_type = DT_DIR;
>+      else
>+        de->d_type = DT_REG;
>+    }
>+#endif
>+

This is just checking all of the Windows types but none of the Cygwin
types.  Shouldn't it be checking for devices, fifos, and symlinks?

>diff --git a/winsup/cygwin/include/sys/dirent.h b/winsup/cygwin/include/sys/dirent.h
>index 41bfcc1..d782e58 100644
>--- a/winsup/cygwin/include/sys/dirent.h
>+++ b/winsup/cygwin/include/sys/dirent.h
>@@ -18,11 +18,17 @@
>
> #pragma pack(push,4)
> #if defined(__INSIDE_CYGWIN__) || defined (__CYGWIN_USE_BIG_TYPES__)
>+#define _DIRENT_HAVE_D_TYPE
> struct dirent
> {
>   long __d_version; /* Used internally */
>   __ino64_t d_ino;
>+#ifdef _DIRENT_HAVE_D_TYPE
>+  unsigned char d_type;
>+  unsigned char __d_unused1[3];
>+#else
>   __uint32_t __d_unused1;
>+#endif

There is even less reason to define and use _DIRENT_HAVE_D_TYPE here.

Why not just define d_type as a __uint32_t?  We don't need to keep the
__d_unused1 around.

cgf
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Add dirent.d_type support to Cygwin 1.7 ?

Christian Franke
Christopher Faylor wrote:

> On Wed, Nov 26, 2008 at 10:24:14PM +0100, Christian Franke wrote:
>  
> ...
>>   de->d_ino = 0;
>> +#ifdef _DIRENT_HAVE_D_TYPE
>> +  de->d_type = DT_UNKNOWN;
>> +#endif
>> +  memset (&de->__d_unused1, 0, sizeof (de->__d_unused1));
>> +
>>    
>
> I don't see a need for a conditional here.  If this is added Cygwin
> supports d_type.
>
>  

OK.


> ...
>>
>> +#ifdef _DIRENT_HAVE_D_TYPE
>> +  /* Set d_type if type can be determined from file attributes.
>> +     FILE_ATTRIBUTE_SYSTEM ommitted to leave DT_UNKNOWN for old symlinks.
>> +     For new symlinks, d_type will be reset to DT_UNKNOWN below.  */
>> +  if (attr &&
>> +      !(attr & ~( FILE_ATTRIBUTE_NORMAL
>> +                | FILE_ATTRIBUTE_READONLY
>> +                | FILE_ATTRIBUTE_ARCHIVE
>> +                | FILE_ATTRIBUTE_HIDDEN
>> +                | FILE_ATTRIBUTE_COMPRESSED
>> +                | FILE_ATTRIBUTE_ENCRYPTED
>> +                | FILE_ATTRIBUTE_SPARSE_FILE
>> +                | FILE_ATTRIBUTE_NOT_CONTENT_INDEXED
>> +                | FILE_ATTRIBUTE_DIRECTORY)))
>> +    {
>> +      if (attr & FILE_ATTRIBUTE_DIRECTORY)
>> +        de->d_type = DT_DIR;
>> +      else
>> +        de->d_type = DT_REG;
>> +    }
>> +#endif
>> +
>>    
>
> This is just checking all of the Windows types but none of the Cygwin
> types.  Shouldn't it be checking for devices, fifos, and symlinks?
>  

D_type should only be set to the actual type if this info is available
at low cost. This is the case for files/dirs, but not for e.g. Cygwin
symlinks. Therefore, DT_UNKNOWN is returned instead and the app must
call stat() if this info is required.

To speed up typical 'find' and 'ls -R' operations, it is IMO enough to
handle the most common filesystem types (for now).


>> diff --git a/winsup/cygwin/include/sys/dirent.h b/winsup/cygwin/include/sys/dirent.h
>> index 41bfcc1..d782e58 100644
>> --- a/winsup/cygwin/include/sys/dirent.h
>> +++ b/winsup/cygwin/include/sys/dirent.h
>> @@ -18,11 +18,17 @@
>>
>> #pragma pack(push,4)
>> #if defined(__INSIDE_CYGWIN__) || defined (__CYGWIN_USE_BIG_TYPES__)
>> +#define _DIRENT_HAVE_D_TYPE
>> struct dirent
>> {
>>   long __d_version; /* Used internally */
>>   __ino64_t d_ino;
>> +#ifdef _DIRENT_HAVE_D_TYPE
>> +  unsigned char d_type;
>> +  unsigned char __d_unused1[3];
>> +#else
>>   __uint32_t __d_unused1;
>> +#endif
>>    
>
> There is even less reason to define and use _DIRENT_HAVE_D_TYPE here.
>
> Why not just define d_type as a __uint32_t?  We don't need to keep the
> __d_unused1 around.
>
>  

_DIRENT_HAVE_D_TYPE and 'unsigned char d_type' are the same under Linux:
http://www.kernel.org/doc/man-pages/online/pages/man3/readdir.3.html


Christian

Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Add dirent.d_type support to Cygwin 1.7 ?

Corinna Vinschen-2
Hi Christian,

On Nov 27 00:12, Christian Franke wrote:

> Christopher Faylor wrote:
>> ...
>>>
>>> +#ifdef _DIRENT_HAVE_D_TYPE
>>> +  /* Set d_type if type can be determined from file attributes.
>>> +     FILE_ATTRIBUTE_SYSTEM ommitted to leave DT_UNKNOWN for old
>>> symlinks.
>>> +     For new symlinks, d_type will be reset to DT_UNKNOWN below.  */
>>> +  if (attr &&
>>> +      !(attr & ~( FILE_ATTRIBUTE_NORMAL
>>> +                | FILE_ATTRIBUTE_READONLY
>>> +                | FILE_ATTRIBUTE_ARCHIVE
>>> +                | FILE_ATTRIBUTE_HIDDEN
>>> +                | FILE_ATTRIBUTE_COMPRESSED
>>> +                | FILE_ATTRIBUTE_ENCRYPTED
>>> +                | FILE_ATTRIBUTE_SPARSE_FILE
>>> +                | FILE_ATTRIBUTE_NOT_CONTENT_INDEXED
>>> +                | FILE_ATTRIBUTE_DIRECTORY)))

I understand why you omit FILE_ATTRIBUTE_REPARSE_POINT in this attribute
list but what about FILE_ATTRIBUTE_OFFLINE, FILE_ATTRIBUTE_TEMPORARY or,
FWIW, any other new attributes which will be created in later Windows
versions?  Shouldn't this condition test positively instead like, say,

  !(attr & (FILE_ATTRIBUTE_REPARSE_POINT | FILE_ATTRIBUTE_DEVICE))

I must admit I never saw the FILE_ATTRIBUTE_DEVICE attribute actually
set anywhere...

>> This is just checking all of the Windows types but none of the Cygwin
>> types.  Shouldn't it be checking for devices, fifos, and symlinks?
>
> D_type should only be set to the actual type if this info is available at
> low cost. This is the case for files/dirs, but not for e.g. Cygwin
> symlinks. Therefore, DT_UNKNOWN is returned instead and the app must call
> stat() if this info is required.
>
> To speed up typical 'find' and 'ls -R' operations, it is IMO enough to
> handle the most common filesystem types (for now).

Yeah, without OS support we have no cheap way to recognize other
filetypes.  As the readir man page says, "all applications must properly
handle a return of DT_UNKNOWN."


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Add dirent.d_type support to Cygwin 1.7 ?

Christian Franke
Hi Corinna,

Corinna Vinschen wrote:

> > > ...
> > > >
> > > > +#ifdef _DIRENT_HAVE_D_TYPE
> > > > +  /* Set d_type if type can be determined from file attributes.
> > > > +     FILE_ATTRIBUTE_SYSTEM ommitted to leave DT_UNKNOWN for old
> > > > symlinks.
> > > > +     For new symlinks, d_type will be reset to DT_UNKNOWN
> > > > below.  */ +  if (attr &&
> > > > +      !(attr & ~( FILE_ATTRIBUTE_NORMAL
> > > > +                | FILE_ATTRIBUTE_READONLY
> > > > +                | FILE_ATTRIBUTE_ARCHIVE
> > > > +                | FILE_ATTRIBUTE_HIDDEN
> > > > +                | FILE_ATTRIBUTE_COMPRESSED
> > > > +                | FILE_ATTRIBUTE_ENCRYPTED
> > > > +                | FILE_ATTRIBUTE_SPARSE_FILE
> > > > +                | FILE_ATTRIBUTE_NOT_CONTENT_INDEXED
> > > > +                | FILE_ATTRIBUTE_DIRECTORY)))
> > > >
>
> I understand why you omit FILE_ATTRIBUTE_REPARSE_POINT in this
> attribute list but what about FILE_ATTRIBUTE_OFFLINE,
> FILE_ATTRIBUTE_TEMPORARY or, FWIW, any other new attributes which will
> be created in later Windows versions?
>

FILE_ATTRIBUTE_TEMPORARY and _OFFLINE should be added.


> Shouldn't this condition test positively instead like, say,
>
> !(attr & (FILE_ATTRIBUTE_REPARSE_POINT | FILE_ATTRIBUTE_DEVICE))
>

If any undocumented flag is set, then DT_UNKNOWN should be returned. MS
might invent new flags, at least on the ntdll layer.


> I must admit I never saw the FILE_ATTRIBUTE_DEVICE attribute actually
> set anywhere...
>

Meantime, I found FILE_ATTRIBUTE_VALID_FLAGS in winnt.h. It does not
include FILE_ATTRIBUTE_DEVICE.

I would suggest the following logic:

if (attr)
{
  if (attr & ~FILE_ATTRIBUTE_VALID_FLAGS)
    {
      /* undocumented flag: DT_UNKNOWN
         Probably print a warning once:
        "... please inform cygwin at cygwin.com" */
    }
  else if (!(attr & (FILE_ATTRIBUTE_SYSTEM
                    |FILE_ATTRIBUTE_REPARSE_POINT))
    {
      /* DT_REG or DT_DIR */
    }
  else
    /* possible old symlink or something special: DT_UNKNOWN */;
}

Christian



Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Add dirent.d_type support to Cygwin 1.7 ?

Corinna Vinschen-2
On Nov 27 11:38, Christian Franke wrote:

> Hi Corinna,
>
> Corinna Vinschen wrote:
> > Shouldn't this condition test positively instead like, say,
> >
> > !(attr & (FILE_ATTRIBUTE_REPARSE_POINT | FILE_ATTRIBUTE_DEVICE))
> >
>
> If any undocumented flag is set, then DT_UNKNOWN should be returned. MS
> might invent new flags, at least on the ntdll layer.

Hmm.  That's the question.  Should we treat new flags as being harmless,
or should we treat them as dangerous?

For instance, if we had done that years ago, before FILE_ATTRIBUTE_ENCRYPTED
had been invented, we would have set all encrypted files to DT_UNKNOWN
later on, until Cygwin would have been rebuilt with a newer winnt.h.

OTOH, DT_UNKNOWN is practically nothing worth to get headaches about.
It just potentially slows down find and ls to the state before inventing
d_type.  So, yeah, I guess it's ok to treat new attributes as
potentially dangerous here.

> Meantime, I found FILE_ATTRIBUTE_VALID_FLAGS in winnt.h. It does not
> include FILE_ATTRIBUTE_DEVICE.
>
> I would suggest the following logic:
>
> if (attr)
> {
>   if (attr & ~FILE_ATTRIBUTE_VALID_FLAGS)
>     {
>       /* undocumented flag: DT_UNKNOWN
>          Probably print a warning once:
>         "... please inform cygwin at cygwin.com" */
>     }
>   else if (!(attr & (FILE_ATTRIBUTE_SYSTEM
>                     |FILE_ATTRIBUTE_REPARSE_POINT))
>     {
>       /* DT_REG or DT_DIR */
>     }
>   else
>     /* possible old symlink or something special: DT_UNKNOWN */;
> }

The logic sounds ok to me.  I just don't think we need a warning
and the condition could be simplified accordingly.


Thanks,
Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Add dirent.d_type support to Cygwin 1.7 ?

Christian Franke
Corinna Vinschen wrote:
> The logic sounds ok to me.  I just don't think we need a warning
> and the condition could be simplified accordingly.
>
>  

New patch below. Conditionals removed as suggested by cgf.

Define of _DIRENT_HAVE_D_TYPE still there - google code search finds
several projects using this instead of a ./configure check.


2008-11-27  Christian Franke  <[hidden email]>

        * dir.cc (readdir_worker): Initialize dirent.d_type and __d_unused1.
        * fhandler_disk_file.cc (fhandler_disk_file::readdir_helper):
        Set dirent.d_type based on FILE_ATTRIBUTE_*.
        * include/sys/dirent.h: Define _DIRENT_HAVE_D_TYPE.
        (struct dirent): Add d_type. Adjust __d_unused1 size to preserve layout.
        [_DIRENT_HAVE_D_TYPE]: Enable DT_* declarations.



Christian


PS: find is not as smart as expected: 'find /path -type d' calls lstat()
for each entry, even if d_type != DT_UNKNOWN.
So 'find /path' is 2-3 times faster than 'find /path -type d'.


diff --git a/winsup/cygwin/dir.cc b/winsup/cygwin/dir.cc
index 30662e6..2b9125f 100644
--- a/winsup/cygwin/dir.cc
+++ b/winsup/cygwin/dir.cc
@@ -93,6 +93,9 @@ readdir_worker (DIR *dir, dirent *de)
     }
 
   de->d_ino = 0;
+  de->d_type = DT_UNKNOWN;
+  memset (&de->__d_unused1, 0, sizeof (de->__d_unused1));
+
   int res = ((fhandler_base *) dir->__fh)->readdir (dir, de);
 
   if (res == ENMFILE)
diff --git a/winsup/cygwin/fhandler_disk_file.cc b/winsup/cygwin/fhandler_disk_file.cc
index c388a13..ac7ee2e 100644
--- a/winsup/cygwin/fhandler_disk_file.cc
+++ b/winsup/cygwin/fhandler_disk_file.cc
@@ -1677,6 +1677,20 @@ fhandler_disk_file::readdir_helper (DIR *dir, dirent *de, DWORD w32_err,
       dir->__flags &= ~dirent_set_d_ino;
     }
 
+  /* Set d_type if type can be determined from file attributes.
+     FILE_ATTRIBUTE_SYSTEM ommitted to leave DT_UNKNOWN for old symlinks.
+     For new symlinks, d_type will be reset to DT_UNKNOWN below.  */
+  if (attr &&
+      !(attr & (  ~FILE_ATTRIBUTE_VALID_FLAGS
+ | FILE_ATTRIBUTE_SYSTEM
+ | FILE_ATTRIBUTE_REPARSE_POINT)))
+    {
+      if (attr & FILE_ATTRIBUTE_DIRECTORY)
+ de->d_type = DT_DIR;
+      else
+ de->d_type = DT_REG;
+    }
+
   /* Check for directory reparse point.  These are potential volume mount
      points which have another inode than the underlying directory. */
   if ((attr & (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_REPARSE_POINT))
@@ -1728,7 +1742,10 @@ fhandler_disk_file::readdir_helper (DIR *dir, dirent *de, DWORD w32_err,
     }
   path_conv fpath (&fbuf, PC_SYM_NOFOLLOW);
   if (fpath.issymlink () || fpath.is_fs_special ())
-    fname->Length -= 4 * sizeof (WCHAR);
+    {
+      fname->Length -= 4 * sizeof (WCHAR);
+      de->d_type = DT_UNKNOWN;
+    }
  }
     }
 
diff --git a/winsup/cygwin/include/sys/dirent.h b/winsup/cygwin/include/sys/dirent.h
index 41bfcc1..451c802 100644
--- a/winsup/cygwin/include/sys/dirent.h
+++ b/winsup/cygwin/include/sys/dirent.h
@@ -18,11 +18,13 @@
 
 #pragma pack(push,4)
 #if defined(__INSIDE_CYGWIN__) || defined (__CYGWIN_USE_BIG_TYPES__)
+#define _DIRENT_HAVE_D_TYPE
 struct dirent
 {
   long __d_version; /* Used internally */
   __ino64_t d_ino;
-  __uint32_t __d_unused1;
+  unsigned char d_type;
+  unsigned char __d_unused1[3];
   __uint32_t __d_internal1;
   char d_name[NAME_MAX + 1];
 };
@@ -77,7 +79,7 @@ int scandir (const char *__dir,
      int (*compar) (const struct dirent **, const struct dirent **));
 
 int alphasort (const struct dirent **__a, const struct dirent **__b);
-#if 0  /* these make no sense in the absence of d_type */
+#ifdef _DIRENT_HAVE_D_TYPE
 /* File types for `d_type'.  */
 enum
 {
@@ -104,6 +106,6 @@ enum
 /* Convert between stat structure types and directory types.  */
 # define IFTODT(mode) (((mode) & 0170000) >> 12)
 # define DTTOIF(dirtype)        ((dirtype) << 12)
-#endif /* #if 0 */
+#endif /* _DIRENT_HAVE_D_TYPE */
 #endif /* _POSIX_SOURCE */
 #endif /*_SYS_DIRENT_H*/
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Add dirent.d_type support to Cygwin 1.7 ?

Christopher Faylor-8
On Thu, Nov 27, 2008 at 10:41:56PM +0100, Christian Franke wrote:

> Corinna Vinschen wrote:
>> The logic sounds ok to me.  I just don't think we need a warning and the
>> condition could be simplified accordingly.
>>
>>  
>
> New patch below. Conditionals removed as suggested by cgf.
>
> Define of _DIRENT_HAVE_D_TYPE still there - google code search finds
> several projects using this instead of a ./configure check.
>
>
> 2008-11-27  Christian Franke  <[hidden email]>
>
> * dir.cc (readdir_worker): Initialize dirent.d_type and __d_unused1.
> * fhandler_disk_file.cc (fhandler_disk_file::readdir_helper):
> Set dirent.d_type based on FILE_ATTRIBUTE_*.
> * include/sys/dirent.h: Define _DIRENT_HAVE_D_TYPE.
> (struct dirent): Add d_type. Adjust __d_unused1 size to preserve layout.
> [_DIRENT_HAVE_D_TYPE]: Enable DT_* declarations.

If Corinna's ok with this then so am I.

cgf
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Add dirent.d_type support to Cygwin 1.7 ?

Corinna Vinschen-2
On Nov 27 21:15, Christopher Faylor wrote:

> On Thu, Nov 27, 2008 at 10:41:56PM +0100, Christian Franke wrote:
> > Corinna Vinschen wrote:
> >> The logic sounds ok to me.  I just don't think we need a warning and the
> >> condition could be simplified accordingly.
> >>
> >>  
> >
> > New patch below. Conditionals removed as suggested by cgf.
> >
> > Define of _DIRENT_HAVE_D_TYPE still there - google code search finds
> > several projects using this instead of a ./configure check.
> >
> >
> > 2008-11-27  Christian Franke  <[hidden email]>
> >
> > * dir.cc (readdir_worker): Initialize dirent.d_type and __d_unused1.
> > * fhandler_disk_file.cc (fhandler_disk_file::readdir_helper):
> > Set dirent.d_type based on FILE_ATTRIBUTE_*.
> > * include/sys/dirent.h: Define _DIRENT_HAVE_D_TYPE.
> > (struct dirent): Add d_type. Adjust __d_unused1 size to preserve layout.
> > [_DIRENT_HAVE_D_TYPE]: Enable DT_* declarations.
>
> If Corinna's ok with this then so am I.

Yep.  Applied with just a minor change to the ChangeLog entry.


Thanks,
Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Add dirent.d_type support to Cygwin 1.7 ?

Christian Franke
Corinna Vinschen wrote:
> On Nov 27 21:15, Christopher Faylor wrote:
>  
>> ...
>> If Corinna's ok with this then so am I.
>>    
>
> Yep.  Applied with just a minor change to the ChangeLog entry.
>
>  

Thanks.

Attn maintainers:
If package with dirent.d_type support is rebuild with new sys/dirent.h,
it is no longer backward compatible with older Cygwin releases. This is
IMO no problem for packages rebuild for 1.7.

If desired, this can be fixed by initializing d_type after opendir:

   DIR * dir = opendir(path);
   if (!dir) {...}
+#if defined(__CYGWIN__) && defined(_DIRENT_HAVE_D_TYPE)
+  dir->__d_dirent->d_type = DT_UNKNOWN;
+#endif


Probably OT: Current CVS does not work for me, bash fails when first
command is run:

[C:\cygwin-1.7\bin].\bash
bash-3.2$ ./true
    419 [sig] bash 3016 _cygtls::handle_exceptions: Exception:
STATUS_ACCESS_VIOLATION
   1032 [sig] bash 3016 open_stackdumpfile: Dumping stack trace to
bash.exe.stackdump

Using exception.cc 1.326 instead of 1.327 fixes the problem.

Christian

Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Add dirent.d_type support to Cygwin 1.7 ?

Christopher Faylor-8
On Fri, Nov 28, 2008 at 10:32:55PM +0100, Christian Franke wrote:
>Attn maintainers:
>If package with dirent.d_type support is rebuild with new sys/dirent.h,
>it is no longer backward compatible with older Cygwin releases.  This
>is IMO no problem for packages rebuild for 1.7.

We never guarantee this so this isn't a big deal.  This happens every
time we add a new function to the DLL.

In fact, we should have bumped the api minor number in
include/cygwin/version.h.  I've just checked in a change which does
that.

cgf