Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

fs: ifx uv_fs_readdir_next reported types #1421

Merged
merged 1 commit into from
Aug 18, 2014
Merged

fs: ifx uv_fs_readdir_next reported types #1421

merged 1 commit into from
Aug 18, 2014

Conversation

saghul
Copy link
Contributor

@saghul saghul commented Aug 14, 2014

Some systems (hello SunOS!) don't have thr d_type field on struct
dirent, so mark them as UV_DIRENT_UNKNOWN, also mark anything not a
directory or a file as unknown.

Jenkins, give me some good news!

@tjfontaine
Copy link
Contributor

I think we can do better at checking if d_type is available instead of only sunos here, there is at least a glibc macro for it.

Relatedly, it seems like we should be reporting more than just FILE, DIRECTORY, and UNKNOWN . The interface can report more (FS supported) including CHR, BLK, FIFO, LNK, and SOCK -- for node we can report all of that information from stat. Since the design principle of this interface is to reduce the need to incur the subsequent stat call we should make our best effort to translate all of the types.

I know that the windows implementation is slightly deficient in what it can report currently, but according to fs__stat_handle we can at least detect LNK on that platform. @orangemocha OOC are there ways to get more information for the kind of file, or have we reached our effective limit here?

@saghul
Copy link
Contributor Author

saghul commented Aug 14, 2014

What glibc macro is that? I checked musl, but I may have missed it. I'll also check what BSDs do, just in case.

@tjfontaine
Copy link
Contributor

To quote the die.net manpage

Only the fields d_name and d_ino are specified in POSIX.1-2001. The remaining fields are available on many, but not all systems. Under glibc, programs can check for the availability of the fields not defined in POSIX.1 by testing whether the macros _DIRENT_HAVE_D_NAMLEN, _DIRENT_HAVE_D_RECLEN, _DIRENT_HAVE_D_OFF, or _DIRENT_HAVE_D_TYPE are defined

if (dent->d_type == UV__DT_DIR)
ent->type = UV_DIRENT_DIR;
else
else if (dent->d_type == UV__DT_FILE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a second thought, I don't really enjoy this line. Could we do ifdef UV__DT_FILE here?

It is very painful to polyfill this, if the most of the file types are unknowns. In fact, the most useful information is dir/not-dir here, and with your patch we are loosing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'd even try to polyfill it on solaris. @tjfontaine are there any way, except stat to get what we need here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me check what we have on the libcs supporting dirent types. IMHO doing stat is overkill. I'd rather say unknown since we don't actually know and let the user stat if he wants to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok...

@saghul
Copy link
Contributor Author

saghul commented Aug 15, 2014

So, about the Unix side: I checked a few libcs:

  • glibc: several HAVE_DIRENT_XX macros, has several DT_ types
  • musl: has several DT_ types
  • bionic: has several DT_ types
  • freeebsd: has several DT_ types
  • openbsd: has several DT_ types
  • osx: has several DT_ types
  • sunos: (didn't check) it apparently doesn't even have the type

So, I'd say the base can be this:

#if defined(DT_UNKNOWN)
# define HAVE_DIRENT_TYPES
...
#endif 

Any implementation which support dirent types needs to have a DT_UNKNOWN one, so we can use that to detect support.

Then, for each type we could do this:

#if defined(DT_REG)
# define UV__DT_FILE DT_REG
#else 
# define UV__DT_FILE -1
#endif 

#if defined(DT_DIR)
# define UV__DT_DIR DT_DIR
#else 
# define UV__DT_DIR -2
#endif 

and so on. Since those constants are always possitive, but our polyfills negative, we can have the if-else-if there just fine, and the last else would set the type to UV_DIRENT_UNKNOWN. Then we can use ifdef HAVE_DIRENT_TYPES to either do the checks or always use unknown (like this patch does).

Thoughts?

/cc @indutny @txdv (since you were also looking into this yesterday)

@indutny
Copy link
Contributor

indutny commented Aug 15, 2014

Makes sense.

@saghul
Copy link
Contributor Author

saghul commented Aug 15, 2014

I just added support for links on both Unix and Windows. This is the complete list of types:


DT_UNKNOWN
    The type is unknown. Only some filesystems have full support to return the type of the file, others might always return this value.
DT_REG
    A regular file.
DT_DIR
    A directory.
DT_FIFO
    A named pipe, or FIFO. See FIFO Special Files.
DT_SOCK
    A local-domain socket.
DT_CHR
    A character device.
DT_BLK
    A block device.
DT_LNK
    A symbolic link. 

So we are missing DT_FIFO, DT_SOCK, DT_CHR and DT_BLK on Unices (which support them). I guess we also want those, right? On Windows a thing can only be a folder, a file or a link, FWIW.

@saghul
Copy link
Contributor Author

saghul commented Aug 15, 2014

Ok, that makes it all types. I used longer names, because unlike in the 80s, characters don't cost money.

@txdv
Copy link
Contributor

txdv commented Aug 15, 2014

Posting this again.

http://www.gnu.org/software/libc/manual/html_node/Directory-Entries.html

http://msdn.microsoft.com/en-us/library/windows/desktop/gg258117(v=vs.85).aspx

I thinks this is a good change, we should return as much information as possible.

The other windows values though seem not to be very interesting.

@saghul
Copy link
Contributor Author

saghul commented Aug 15, 2014

Yep, that's where I took the data from.

@orangemocha
Copy link

Great @saghul, so all file types can be detected on Windows!
On Windows we actually get more attributes as mentioned by @txdv . I think some of those could be interesting, prominently FILE_ATTRIBUTE_READONLY, but also FILE_ATTRIBUTE_HIDDEN and FILE_ATTRIBUTE_SYSTEM. But it sounds like readdir doesn't provide that information.

@saghul
Copy link
Contributor Author

saghul commented Aug 15, 2014

I think some of those could be interesting, prominently FILE_ATTRIBUTE_READONLY, but also FILE_ATTRIBUTE_HIDDEN and FILE_ATTRIBUTE_SYSTEM. But it sounds like readdir doesn't provide that information.

Exactly, there is no match in the data we get in readdir / scandir. Also, being readonly or hidden is not really a file "type" it's more like an attribute of the file itself. Not sure if we can report more on uv_fs_stat, but that's another issue :-)

I squashed the commits and force pushed.

Support all possible types on Unix, and files, directories and links on
Windows. Some systems (hello SunOS!) don't have the d_type field on struct
dirent, so mark them as UV_DIRENT_UNKNOWN.
@piscisaureus
Copy link

In windows you can get a lot of information about directory entries (see http://msdn.microsoft.com/en-us/library/windows/hardware/ff540303%28v=vs.85%29.aspx).

But it's not possible to see symbolic links - they will be reported either as files or as directories.

@piscisaureus
Copy link

Contradicting myself:

But it's not possible to see symbolic links - they will be reported either as files or as directories.

Actually, on the cygwin mailing list, I read this:

(**) For a reparse point, the EaSize member in FILE_ID_BOTH_DIR_INFORMATION
     or FILE_BOTH_DIRECTORY_INFORMATION is set to the ReparseTag.

@piscisaureus
Copy link

@saghul
Copy link
Contributor Author

saghul commented Aug 15, 2014

@piscisaureus on uv_fs_stat, we set the file mode to a link if it's a reparse point: https://github.com/joyent/libuv/blob/master/src/win/fs.c#L960 I did the same here. Is it actually incorrect?

@saghul
Copy link
Contributor Author

saghul commented Aug 15, 2014

In windows you can get a lot of information about directory entries (see http://msdn.microsoft.com/en-us/library/windows/hardware/ff540303%28v=vs.85%29.aspx).

I see. However, the types we don't support on Windows (remember we are on readdir / scandir) doen't seem to apply: character device, block device, fifo, local domain socket. Do they?

@piscisaureus
Copy link

@piscisaureus on uv_fs_stat, we set the file mode to a link if it's a reparse point: https://github.com/joyent/libuv/blob/master/src/win/fs.c#L960 I did the same here. Is it actually incorrect?

That's correct. This means we also report "link" for mount points, but those behave very similar to symlinks on windows anyway (technically they're just directory junctions).

I see. However, the types we don't support on Windows (remember we are on readdir / scandir) doen't seem to apply: character device, block device, fifo, local domain socket. Do they?

Those type of devices do exist on windows, but they cannot live in ordinary directories. Pipes all live in \\.\pipe, block/character devices (console, serial port, raw disk access) are not currently accesible with libuv. Fifo's don't exist on windows.

@saghul
Copy link
Contributor Author

saghul commented Aug 15, 2014

@piscisaureus Kewl! So, it looks like we have all our bases covered for uv_fs_readdir then: anything supported on Unix and directories, files and links on Windows.

@orangemocha
Copy link

@piscisaureus I think that API is only for device drivers.

@tjfontaine
Copy link
Contributor

@saghul this looks like a much more improved, thanks!

@piscisaureus
Copy link

@orangemocha the API can be used from user land. The second link is a spec for driver implementors indeed, we are on the "consuming" end.

@saghul saghul merged commit df8ab50 into joyent:master Aug 18, 2014
@saghul
Copy link
Contributor Author

saghul commented Aug 18, 2014

Landed, thanks to everyone who helped!

@saghul saghul deleted the fix-readdir branch September 11, 2014 19:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants