764436 - [OS.File] Directory traversal

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Attachment: Possible API for file traversal

We need a directory traversal mechanism for OS.File. As usual, we will start with a worker thread-only synchronous version.

Attaching a possible API.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 632721 [details]
Possible API for file traversal

Oops, my bad.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 632721 [details]
Possible API for file traversal

This looks pretty good to me.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: Directory-related constants

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Windows front-end

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: Unix front-end

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Windows back-end

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Unix back-end

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 633152 [details] [diff] [review]
Unix front-end

Taras, could you tell me what you think of this API?

Comment 10

[(dormant account)]

Comment on attachment 633152 [details] [diff] [review]
Unix front-end

i think 
if (!this._dir) return; checks are redundant. Things will just fail if you attempt to do stuff after close() anyway. But I don't have strong feelings about this

+       /**
+        * |true| if the entry is a special directory, |false| otherwise
+        */
+       get isSpecialDir() {
+         return this.name == "." || this.name == "..";
+       }, 


I think we should just skip . and .. in directory listings and get rid of this.

+       get name() {
+         delete this.name;
+         let name = this._entry.d_name.readString();
+         Object.defineProperty(this, "name", {value: name});
+         return name;
+       }

What's going on here, is this meant to be a lazy getter?

r- because I want to see '.' & '..' removed.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

(In reply to Taras Glek (:taras) from comment #10)
> Comment on attachment 633152 [details] [diff] [review]
> Unix front-end
> 
> i think 
> if (!this._dir) return; checks are redundant. Things will just fail if you
> attempt to do stuff after close() anyway. But I don't have strong feelings
> about this
> 
> +       /**
> +        * |true| if the entry is a special directory, |false| otherwise
> +        */
> +       get isSpecialDir() {
> +         return this.name == "." || this.name == "..";
> +       }, 
> 
> 
> I think we should just skip . and .. in directory listings and get rid of
> this.

Ok.

> +       get name() {
> +         delete this.name;
> +         let name = this._entry.d_name.readString();
> +         Object.defineProperty(this, "name", {value: name});
> +         return name;
> +       }
> 
> What's going on here, is this meant to be a lazy getter?

This is a lazy getter compatible with "use strict".

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Attachment: Windows front-end

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Attachment: Unix front-end

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: Unix back-end

Here we go. The new version filters out "." and "..", and can return the full name of the file examined.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Attachment: Windows front-end

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Attachment: Windows back-end

Comment 17

[(dormant account)]

Comment on attachment 634824 [details] [diff] [review]
Unix back-end

How does .OSFILE_OFFSET_D_TYPE get defined?

Comment 18

[(dormant account)]

Comment on attachment 634824 [details] [diff] [review]
Unix back-end

This magic needs heavy commenting
+
+       {
+         let dirent_struct = [];
+         if (OS.Constants.libc.OSFILE_OFFSET_D_TYPE != 0) {
+           dirent_struct.push({padding_before_d_type: ctypes.ArrayType(ctypes.uint8_t, OS.Constants.libc.OSFILE_OFFSET_D_TYPE)});
+         }
+         dirent_struct.push({d_type: ctypes.uint8_t});
+         if (OS.Constants.libc.OSFILE_OFFSET_D_NAME > OS.Constants.libc.OSFILE_OFFSET_D_TYPE + 1) {
+           dirent_struct.push({padding_before_d_name: ctypes.ArrayType(ctypes.uint8_t, OS.Constants.libc.OSFILE_OFFSET_D_NAME - OS.Constants.libc.OSFILE_OFFSET_D_TYPE - 1)});
+         }
+         dirent_struct.push({d_name:ctypes.ArrayType(ctypes.char, OS.Constants.libc.OSFILE_SIZEOF_D_NAME)});
+       Types.dirent =
+         new Type("dirent",
+           ctypes.StructType("dirent", dirent_struct));
+         LOG("dirent is: " + Types.dirent.implementation.toSource());
+       }


+       UnixFile.readdir =
+         declareFFI("readdir$INODE64", ctypes.default_abi,
+                     /*return*/Types.null_or_dirent_ptr,
+                     /*dir*/   Types.DIR.in_ptr) // For MacOS X
+        || declareFFI("readdir", ctypes.default_abi,
+                    /*return*/Types.null_or_dirent_ptr,
+                    /*dir*/   Types.DIR.in_ptr); // Other Unices

really? Can we either flip this logic so it works on normal unix without ifdefing or use the C preprocessor on this file(i prefer this option). This might work better for above dirent magic too.

Comment 19

[(dormant account)]

Comment on attachment 634824 [details] [diff] [review]
Unix back-end

forgot to r- in previous comment

Comment 20

[David Teller [:Yoric] - still alive but not very active]

(In reply to Taras Glek (:taras) from comment #17)
> Comment on attachment 634824 [details] [diff] [review]
> Unix back-end
> 
> How does .OSFILE_OFFSET_D_TYPE get defined?

It comes from patch "Directory-related constants". It is an |offsetof|.

> This magic needs heavy commenting

Definitely.

> really? Can we either flip this logic so it works on normal unix without ifdefing or use the C preprocessor on this file(i prefer this option). This might work better for above dirent magic too.

Yes, really. Finding out the symbol name for MacOS X was lots of fun - they actually use assembly-level tricks to get this compiled.

I personally dislike (a lot) using the C preprocessor on JavaScript, because:
- it completely kills line numbers, hence making debugging much more complicated;
- in our build system, it is actually not the C preprocessor but a much less powerful ersatz that really cannot do much;
- this splits the logic across yet another file (either configure.in or Makefile.in).

Let me produce an alternative, you will tell me if you like it better.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Attachment: Unix back-end

Clarifying and documenting the definition of |dirent|.
Note that we are going to have the same issues with |stat| in bug 766194.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Attachment: Directory-related constants

So, Taras, this is where the constants come from. I have added a little more documentation and macro _DARWIN_FEATURE_64_BIT_INODE to clarify a little the definition of |readdir|/|readdir$INODE64|.

Note that we will use the same macro for |stat|.

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Attachment: Windows back-end

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Attachment: Windows front-end

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Attachment: Unix front-end

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Attachment: Windows front-end

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Attachment: Windows back-end

Comment 28

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Attachment: Unix front-end

Small fix.

Comment 30

[(dormant account)]

David, I likely wont be able to get to these reviews this week. Either ask someone else for review or hold tight :)

Comment 31

[David Teller [:Yoric] - still alive but not very active]

Attachment: Shared code

After some "interesting" debugging, I have done a little refactoring on the definition of |dirent|, so I am attaching two patches for this purpose.

Comment 32

[David Teller [:Yoric] - still alive but not very active]

Attachment: Directory-related constants

Comment 33

[David Teller [:Yoric] - still alive but not very active]

Attachment: Unix back-end

Comment 34

[David Teller [:Yoric] - still alive but not very active]

Attachment: Windows front-end

Comment 35

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 36

[Doug Turner (:dougt)]

Comment on attachment 636677 [details] [diff] [review]
Shared code

Review of attachment 636677 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/osfile_shared.jsm
@@ +417,5 @@
> +       if (!size || size < 0) {
> +         throw new TypeError("HollowStructure expects a (positive) size");
> +       }
> +       this.offset_to_field_info = [];
> +       this.name = name;

Why are some members prefixed with _

@@ +419,5 @@
> +       }
> +       this.offset_to_field_info = [];
> +       this.name = name;
> +       this._size = size;
> +       this._paddings = 0;

why do we need paddings?  Is it singular or plural?

@@ +430,5 @@
> +        * @param {string} name The name of the field.
> +        * @param {CType|Type} type The type of the field.
> +        */
> +       add_field_at: function add_field_at(offset, name, type) {
> +         if (type instanceof Type) {

maybe move below the tests so that you don't assign this on the parameter checking failures.

Comment 37

[David Teller [:Yoric] - still alive but not very active]

(In reply to Doug Turner (:dougt) from comment #36)
> Comment on attachment 636677 [details] [diff] [review]
> Shared code
> 
> Review of attachment 636677 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/osfile_shared.jsm
> @@ +417,5 @@
> > +       if (!size || size < 0) {
> > +         throw new TypeError("HollowStructure expects a (positive) size");
> > +       }
> > +       this.offset_to_field_info = [];
> > +       this.name = name;
> 
> Why are some members prefixed with _

You are right, this is probably not needed.

> @@ +419,5 @@
> > +       }
> > +       this.offset_to_field_info = [];
> > +       this.name = name;
> > +       this._size = size;
> > +       this._paddings = 0;
> 
> why do we need paddings?  Is it singular or plural?

This is simply a counter, to call the first padding field "padding_0", the second "padding_1", etc.

> 
> @@ +430,5 @@
> > +        * @param {string} name The name of the field.
> > +        * @param {CType|Type} type The type of the field.
> > +        */
> > +       add_field_at: function add_field_at(offset, name, type) {
> > +         if (type instanceof Type) {
> 
> maybe move below the tests so that you don't assign this on the parameter
> checking failures.

Good point.

Comment 38

[David Teller [:Yoric] - still alive but not very active]

Attachment: Shared code

Applied feedback.

Comment 39

[(dormant account)]

Comment on attachment 636683 [details] [diff] [review]
Windows front-end

I would prefer s/fileTimeToDate/FILETIME_to_Date/, this way it's more clear which filetime

 throw new File.Error("iter", error) <-- mention FindFirstFile in the error message


Good work

Comment 40

[David Teller [:Yoric] - still alive but not very active]

Attachment: Directory-related constants

Comment 41

[David Teller [:Yoric] - still alive but not very active]

Attachment: Shared code

Comment 42

[David Teller [:Yoric] - still alive but not very active]

Attachment: Unix back-end

Comment 43

[David Teller [:Yoric] - still alive but not very active]

Attachment: Unix front-end

Comment 44

[David Teller [:Yoric] - still alive but not very active]

Attachment: Windows back-end

Comment 45

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 46

[David Teller [:Yoric] - still alive but not very active]

Attachment: Windows front-end

Comment 47

[David Teller [:Yoric] - still alive but not very active]

Attachment: Directory-related constants

Comment 48

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 49

[David Teller [:Yoric] - still alive but not very active]

Attachment: Windows front-end

Comment 50

[David Teller [:Yoric] - still alive but not very active]

Attachment: Windows back-end

Comment 51

[Tim Taubert [:ttaubert] (inactive)]

Will check this in \o/.

Comment 52

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/2e9dac9249f9
https://hg.mozilla.org/integration/fx-team/rev/ba4c17db2345
https://hg.mozilla.org/integration/fx-team/rev/ffb095a61280
https://hg.mozilla.org/integration/fx-team/rev/844c43a98031
https://hg.mozilla.org/integration/fx-team/rev/f4acd66d4d19
https://hg.mozilla.org/integration/fx-team/rev/e96ad4de3feb
https://hg.mozilla.org/integration/fx-team/rev/36a9882bb6c7

Comment 53

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2e9dac9249f9
https://hg.mozilla.org/mozilla-central/rev/ba4c17db2345
https://hg.mozilla.org/mozilla-central/rev/ffb095a61280
https://hg.mozilla.org/mozilla-central/rev/844c43a98031
https://hg.mozilla.org/mozilla-central/rev/f4acd66d4d19
https://hg.mozilla.org/mozilla-central/rev/e96ad4de3feb
https://hg.mozilla.org/mozilla-central/rev/36a9882bb6c7

Comment 54

[Landry Breuil (:gaston)]

Just a heads up, i'll investigate more later, but it seems that the OSFileConstants.cpp commit (https://hg.mozilla.org/mozilla-central/rev/844c43a98031)
broke my builds : http://buildbot.rhaalovely.net/builders/mozilla-central-i386/builds/220/steps/build/logs/stdio

OSFileConstants.cpp:278: error: 'DT_WHT' was not declared in this scope

If i look at our sys/dirent.h (http://fxr.watson.org/fxr/source/sys/dirent.h?v=OPENBSD#L65) it shows that DT_WHT doesnt exist.

If i look at the FreeBSD/NetBSD headers, DT_WHT is here, so i'll see what the standard says wrt those #defines and if it's missing on our side, or if it'd have to be #ifdefed in OSFileConstants.cpp...

Looking at history in our mailing lists, it seems it wont come to sys/dirent.h .. http://old.nabble.com/Re%3A-add-missing-DT_WHT-in-sys-dirent.h-p31216633.html

Finally, note that if i look at a linux manpage for readdir (http://linux.die.net/man/3/readdir) it doesn't mention DT_WHT.

Comment 55

[Landry Breuil (:gaston)]

And after a bit of discussion with our standards peers :

- whiteouts were to support union mounts,  ie the ability to remove a file from a union without removing it from the underlying unioned fs - OpenBSD of course removed support for union mounts ages ago.

- DT_WHT is not standard at all, posix doesn't even standardize *any* of the DT_*

- DT_WHT was only used in return values of readdir2, which was a BSD4ism that existed *just* to support DT_WHT and union filesystems

OS.File only calls plain readdir(), so there's no chance for it to get a value with a DT_WHT type.

So here we have the following choices :
- remove the constant definition for DT_WHT (since it's non-standard and legacy ?)
- wrap it within #ifndef (__OpenBSD__) (ugly!)
- wrap it within #if defined(DT_WHT) (probably the better approach..)

Comment 56

[David Teller [:Yoric] - still alive but not very active]

I think that we should completely remove DT_WHT for the moment, until we have a usage scenario. Can you handle this or would you prefer if I do?

Comment 57

[Landry Breuil (:gaston)]

Attachment: remove unused dt_wth

Sure, here's a patch that fixes the build for me

Comment 58

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 639961 [details] [diff] [review]
remove unused dt_wth

Review of attachment 639961 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
Thanks for the patch!

Comment 59

[Landry Breuil (:gaston)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3fa8a7059b69

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3fa8a7059b69