770538 - [OS.File] Recovering the File from a DirectoryIterator

Status: RESOLVED FIXED

(Core :: Networking: File, enhancement)

Description

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

Implement a getter |file| that returns the file corresponding to a directory iterator. This can be very useful for bug 770501.

Comment 1

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

Not trivial, but this can be an interesting first bug/mentored bug.

Comment 2

[jdev005]

can i be assigned this bug ?

Comment 3

[Mayuresh Raut]

Hello,
I am a Computer Engineering graduate student at NC State University, Raleigh. I would like to work on this bug to get an exposure of working with the open source community.
Thanks,
Mayuresh

(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from comment #1)
> Not trivial, but this can be an interesting first bug/mentored bug.

Comment 4

[Josh Matthews [:jdm]]

Sorry for the delay Mayuresh. Yoric might not be around right now due to vacation, but the code for OS.File is at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/.

Comment 5

[Josh Matthews [:jdm]]

More concretely, you can find the DirectoryIterator code in these files: http://mxr.mozilla.org/mozilla-central/search?string=directoryiterator

Comment 6

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

My apologies, I missed your messages while on vacation.

jdev005, Mayuresh, are you both interested in that bug?

There are two versions of DirectoryIterator: one for Unix http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/osfile_unix_front.jsm#545 and one for Windows http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/osfile_win_front.jsm#449 .

For Windows, getting the file for a directory is disappointingly simple: take the path and open the file using the constants FILE_STAT_MODE and FILE_STAT_OPTIONS.

For Unix, getting the file for a directory is more interesting: take the DIR and apply Posix function |dirfd|.

Do not hesitate to get in touch with me on IRC or ask any question.

Comment 7

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

Actually, let's keep the Windows version for later. For the time being, only the Unix version is interesting.

Comment 8

[Mark Capella [:capella]]

So for this bug, you don't seek to implement a File.prototype.linuxDirectoryIterator (from bug 770501) but more something like "fileFD":

??

  let iterator = new OS.File.DirectoryIterator(foo);
  for (let entry in iterator) {
    if (entry.name == foobar) {
      let buf = new (ctypes.ArrayType(ctypes.char, 4096))();
      buflen = entry.fileFD.read(buf, 4096);

Comment 9

[Mark Capella [:capella]]

Or maybe something like the below (leveraging the fooat() functions) ..? Can you elaborate on the scope / ramp-up plan here (I keep missing you on irc)

let iterator = new OS.File.DirectoryIterator(foo);
for (let entry in iterator) {
  if (entry.name == foobar) {
    let entry_file =
        OS.File.openat(iterator.fileFD, entry.name, {write: true, trunc:true});

Comment 10

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

The exact API is undecided, but yes, the main idea is to leverage |*at()| functions.

The first usage will be speeding-up the following tasks:
- walking through a directory recursively;
- sorting all the elements of a directory by last modification time;
- removing a directory recursively.

Comment 11

[Mitesh Pathak [:mr_pathak]]

Attachment: Ist proposed patch for bug 770538

Comment 12

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

Comment on attachment 670153 [details] [diff] [review]
Ist proposed patch for bug 770538

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

Good patch, although it still requires a little work.

See the comments below. Also, please remove the trailing whitespaces :)

::: toolkit/components/osfile/osfile_unix_back.jsm
@@ +243,5 @@
>  
> +       UnixFile.dirfd =
> +         declareFFI("dirfd", ctypes.default_abi,
> +                    /*return*/ Types.negativeone_or_fd,
> +                    /*dir*/     Types.null_or_DIR_ptr);

Nit: remove one whitespace to align with previous line.

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +635,5 @@
>         this._dir = null;
>       };
>  
>       /**
> +      * Return directory as |File| using |dirfd|

This "using |dirfd|" is not necessary.

@@ +637,5 @@
>  
>       /**
> +      * Return directory as |File| using |dirfd|
> +      */
> +     File.DirectoryIterator.prototype.dirfd = function dirfd() {

As all functions/methods that are specific to Unix, this method should be prefixed with |unix|.

Let's call this |unixAsFile|.

@@ +736,5 @@
>          * The size of the file, in bytes.
>          *
>          * Note that the result may be |NaN| if the size of the file cannot be
>          * represented in JavaScript.
> +        * FIXME |File| created using |OS.File.DirectoryIterator.dirfd| has size = 4096 (for all directories) 

Nit: Wrong place for this comment.

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +561,5 @@
> +  if("dirfd" in OS.File.DirectoryIterator.prototype)
> +  {
> +   ok(true, "testing property |dirfd|");
> +   try{
> +   

What is this |try| doing here?

@@ +563,5 @@
> +   ok(true, "testing property |dirfd|");
> +   try{
> +   
> +   iterator = new OS.File.DirectoryIterator("chrome/toolkit/components/osfile/tests/mochi");
> +   

Please use |OS.Path.join| to construct paths. Also, record the path somewhere to be sure that you use the same in |stat1|.

@@ +566,5 @@
> +   iterator = new OS.File.DirectoryIterator("chrome/toolkit/components/osfile/tests/mochi");
> +   
> +   let dir_file = iterator.dirfd();// return |File|
> +   let stat0=dir_file.stat();
> +   let s0_string= "| "+stat0.unixMode+" | "+stat0.unixOwner+" | "+stat0.unixGroup+" | "+stat0.creationDate+" | "+stat0.lastModificationDate+" | "+stat0.lastAccessDate+" | "+stat0.size+" |";

Since you repeat this operation, you should write a (local) function that converts an information into a string.

let unix_info_to_string = function unix_info_to_string(info) {
   return "|" + ...
};

@@ +574,5 @@
> +   let s1_string= "| "+stat1.unixMode+" | "+stat1.unixOwner+" | "+stat1.unixGroup+" | "+stat1.creationDate+" | "+stat1.lastModificationDate+" | "+stat1.lastAccessDate+" | "+stat1.size+" |";
> +
> +   //status of different directory with its string representation
> +   let stat2=OS.File.stat("chrome/toolkit/components/osfile/tests");
> +   let s2_string= "| "+stat2.unixMode+" | "+stat2.unixOwner+" | "+stat2.unixGroup+" | "+stat2.creationDate+" | "+stat2.lastModificationDate+" | "+stat2.lastAccessDate+" | "+stat2.size+" |";

What is the point of |stat2|?

@@ +577,5 @@
> +   let stat2=OS.File.stat("chrome/toolkit/components/osfile/tests");
> +   let s2_string= "| "+stat2.unixMode+" | "+stat2.unixOwner+" | "+stat2.unixGroup+" | "+stat2.creationDate+" | "+stat2.lastModificationDate+" | "+stat2.lastAccessDate+" | "+stat2.size+" |";
> +
> +
> +   if(stat0.isDir) {

Instead of this |if|, just do a sanity check:

ok(stat0.isDir, "unixAsFile returned a directory");

@@ +580,5 @@
> +
> +   if(stat0.isDir) {
> +    ok(true,"File defined is a Directory ");
> +    //s0_string should be equal to s1_string & different from s2_string
> +    if(s0_string.localeCompare(s1_string)==0 && s0_string.localeCompare(s2_string)!=0) {

|localeCompare|? That looks really very much like overkill. Just use == or (preferably) function |is|. This is JavaScript, it works :)

@@ +581,5 @@
> +   if(stat0.isDir) {
> +    ok(true,"File defined is a Directory ");
> +    //s0_string should be equal to s1_string & different from s2_string
> +    if(s0_string.localeCompare(s1_string)==0 && s0_string.localeCompare(s2_string)!=0) {
> +	ok(true,"test_iter_dir: |File| successfully created using |dirfd|");

Here, just use is:

is(s0_string, s1_string, "unixAsFile returned the correct file");

@@ +591,5 @@
> +   } 
> +   dir_file.close();
> +   iterator.close();
> +   } catch (x) {
> +     ok(false,x.toString()+"\n"+x.stack);

There is already some code doing this |try ... catch ...| for all tests. You can/should get rid of your |try ... catch ...| here.

Comment 13

[Mitesh Pathak [:mr_pathak]]

Attachment: IInd patch for bug 770538

Changes recommended by  David Rajchenbach Teller [:Yoric] applied.

Comment 14

[Mitesh Pathak [:mr_pathak]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #12)

> 
> ::: toolkit/components/osfile/osfile_unix_back.jsm
> @@ +243,5 @@
> >  
> > +       UnixFile.dirfd =
> > +         declareFFI("dirfd", ctypes.default_abi,
> > +                    /*return*/ Types.negativeone_or_fd,
> > +                    /*dir*/     Types.null_or_DIR_ptr);
> 
> Nit: remove one whitespace to align with previous line.

Done

> ::: toolkit/components/osfile/osfile_unix_front.jsm
> @@ +635,5 @@
> >         this._dir = null;
> >       };
> >  
> >       /**
> > +      * Return directory as |File| using |dirfd|
> 
> This "using |dirfd|" is not necessary.

Done 
 
> @@ +637,5 @@
> >  
> >       /**
> > +      * Return directory as |File| using |dirfd|
> > +      */
> > +     File.DirectoryIterator.prototype.dirfd = function dirfd() {
> 
> As all functions/methods that are specific to Unix, this method should be
> prefixed with |unix|.
> 
> Let's call this |unixAsFile|.

Done

> > +        * FIXME |File| created using |OS.File.DirectoryIterator.dirfd| has size = 4096 (for all directories) 
> 
> Nit: Wrong place for this comment.

Removed

> ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
> @@ +561,5 @@
> > +  if("dirfd" in OS.File.DirectoryIterator.prototype)
> > +  {
> > +   ok(true, "testing property |dirfd|");
> > +   try{
> > +   
> 
> What is this |try| doing here?

removed 

> @@ +563,5 @@
> > +   ok(true, "testing property |dirfd|");
> > +   try{
> > +   
> > +   iterator = new OS.File.DirectoryIterator("chrome/toolkit/components/osfile/tests/mochi");
> > +   
> 
> Please use |OS.Path.join| to construct paths. Also, record the path
> somewhere to be sure that you use the same in |stat1|.

Done 

> @@ +566,5 @@
> > +   iterator = new OS.File.DirectoryIterator("chrome/toolkit/components/osfile/tests/mochi");
> > +   
> > +   let dir_file = iterator.dirfd();// return |File|
> > +   let stat0=dir_file.stat();
> > +   let s0_string= "| "+stat0.unixMode+" | "+stat0.unixOwner+" | "+stat0.unixGroup+" | "+stat0.creationDate+" | "+stat0.lastModificationDate+" | "+stat0.lastAccessDate+" | "+stat0.size+" |";
> 
> Since you repeat this operation, you should write a (local) function that
> converts an information into a string.
> 
> let unix_info_to_string = function unix_info_to_string(info) {
>    return "|" + ...
> };

Done

> @@ +574,5 @@
> > +   let s1_string= "| "+stat1.unixMode+" | "+stat1.unixOwner+" | "+stat1.unixGroup+" | "+stat1.creationDate+" | "+stat1.lastModificationDate+" | "+stat1.lastAccessDate+" | "+stat1.size+" |";
> > +
> > +   //status of different directory with its string representation
> > +   let stat2=OS.File.stat("chrome/toolkit/components/osfile/tests");
> > +   let s2_string= "| "+stat2.unixMode+" | "+stat2.unixOwner+" | "+stat2.unixGroup+" | "+stat2.creationDate+" | "+stat2.lastModificationDate+" | "+stat2.lastAccessDate+" | "+stat2.size+" |";
> 
> What is the point of |stat2|?

I used |stat1|- with same dir and |stat2|- with other dir, so that even failure is tested . |stat2 != stat0| were |stat0 = dir_file.stat()|
I have removed |stat2| in next patch

> @@ +577,5 @@
> > +   let stat2=OS.File.stat("chrome/toolkit/components/osfile/tests");
> > +   let s2_string= "| "+stat2.unixMode+" | "+stat2.unixOwner+" | "+stat2.unixGroup+" | "+stat2.creationDate+" | "+stat2.lastModificationDate+" | "+stat2.lastAccessDate+" | "+stat2.size+" |";
> > +
> > +
> > +   if(stat0.isDir) {
> 
> Instead of this |if|, just do a sanity check:
> 
> ok(stat0.isDir, "unixAsFile returned a directory");
> 
> @@ +580,5 @@
> > +
> > +   if(stat0.isDir) {
> > +    ok(true,"File defined is a Directory ");
> > +    //s0_string should be equal to s1_string & different from s2_string
> > +    if(s0_string.localeCompare(s1_string)==0 && s0_string.localeCompare(s2_string)!=0) {
> 
> |localeCompare|? That looks really very much like overkill. Just use == or
> (preferably) function |is|. This is JavaScript, it works :)

Done

> @@ +581,5 @@
> > +   if(stat0.isDir) {
> > +    ok(true,"File defined is a Directory ");
> > +    //s0_string should be equal to s1_string & different from s2_string
> > +    if(s0_string.localeCompare(s1_string)==0 && s0_string.localeCompare(s2_string)!=0) {
> > +	ok(true,"test_iter_dir: |File| successfully created using |dirfd|");
>
> Here, just use is:
> 
> is(s0_string, s1_string, "unixAsFile returned the correct file");

Done 

> @@ +591,5 @@
> > +   } 
> > +   dir_file.close();
> > +   iterator.close();
> > +   } catch (x) {
> > +     ok(false,x.toString()+"\n"+x.stack);
> 
> There is already some code doing this |try ... catch ...| for all tests. You
> can/should get rid of your |try ... catch ...| here.

Removed

Thanks for the feedback,
 Mitesh Pathak

Comment 15

[Mitesh Pathak [:mr_pathak]]

Attachment: 2nd Patch -for bug 770538

Changes recommended by  David Rajchenbach Teller [:Yoric] applied.

Comment 16

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

Comment on attachment 670400 [details] [diff] [review]
2nd Patch -for bug 770538

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

Looks good. You have my r+ provided:
- you remove the useless whitespaces in worker_test_osfile_front.js;
- the tests pass.

Comment 17

[Mitesh Pathak [:mr_pathak]]

Attachment: Bug-770538 patch

Comment 18

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=d53b8198d52b

Comment 19

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

Mmmmhh... this does not work on MacOS X. After investigation, this is because MacOS X defines |dirfd| as a macro. I will add a hack to get this to work.

Comment 20

[Mitesh Pathak [:mr_pathak]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #19)
> Mmmmhh... this does not work on MacOS X. After investigation, this is
> because MacOS X defines |dirfd| as a macro. I will add a hack to get this to
> work.

Thanks for your continuous support

Regards
Mitesh Pathak

Comment 21

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

Comment on attachment 670482 [details] [diff] [review]
Bug-770538 patch

No need to request my feedback again.

Comment 22

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

Attachment: 2. Getting the Mac version to work

Here is a version that should work on a Mac. Could you please take a look, Nathan?

Comment 23

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

TryServer: https://tbpl.mozilla.org/?tree=Try&rev=09dd7fb8fa3a

Comment 24

[Nathan Froyd [:froydnj]]

Comment on attachment 671205 [details] [diff] [review]
2. Getting the Mac version to work

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

::: toolkit/components/osfile/osfile_unix_back.jsm
@@ +168,5 @@
> +         // directly, we need to define it as a hollow structure.
> +         // In principle, we could do this for all platforms. However,
> +         // the exact name of interesting fields varies across platforms,
> +         // which makes it troublesome, so we concentrate on getting this
> +         // to work for as few platforms as possible.

Nit: I think this would be better as:

"On platforms for which we need to access the fields of DIR directly (e.g. because certain functions are implemented as macros), we need to define it as a hollow structure."  Don't pontificate about what would be an interesting theoretical exercise. :)

@@ +268,5 @@
>           declareFFI("dirfd", ctypes.default_abi,
>                      /*return*/ Types.negativeone_or_fd,
> +                    /*dir*/    Types.DIR.in_ptr)
> +         ||
> +         // Under MacOS X, |dirfd| is a macro, we need to reimplement it

Since you didn't refer to specific OSes above, maybe we should unconditionally use the JS-dirfd function when OSFILE_SIZEOF_DIR?

Comment 25

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

(In reply to Nathan Froyd (:froydnj) from comment #24)
> Comment on attachment 671205 [details] [diff] [review]
> 2. Getting the Mac version to work
> 
> Review of attachment 671205 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/osfile_unix_back.jsm
> @@ +168,5 @@
> > +         // directly, we need to define it as a hollow structure.
> > +         // In principle, we could do this for all platforms. However,
> > +         // the exact name of interesting fields varies across platforms,
> > +         // which makes it troublesome, so we concentrate on getting this
> > +         // to work for as few platforms as possible.
> 
> Nit: I think this would be better as:
> 
> "On platforms for which we need to access the fields of DIR directly (e.g.
> because certain functions are implemented as macros), we need to define it
> as a hollow structure."  Don't pontificate about what would be an
> interesting theoretical exercise. :)

Fair enough :)

> 
> @@ +268,5 @@
> >           declareFFI("dirfd", ctypes.default_abi,
> >                      /*return*/ Types.negativeone_or_fd,
> > +                    /*dir*/    Types.DIR.in_ptr)
> > +         ||
> > +         // Under MacOS X, |dirfd| is a macro, we need to reimplement it
> 
> Since you didn't refer to specific OSes above, maybe we should
> unconditionally use the JS-dirfd function when OSFILE_SIZEOF_DIR?

Good idea.

Comment 26

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

Attachment: 2. Getting the Mac version to work, v2

Comment 27

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

Attachment: 2. Getting the Mac version to work, v3

Same code, but without forgetting a negation.

Try: https://tbpl.mozilla.org/?tree=Try&rev=8deb75be993f

Comment 28

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

Attachment: 2. Getting the Mac version to work, v3

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea6edd3749ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/354250ed1e72

Comment 30

[Ed Morley [:emorley]]

Backed out along with several others in order to get inbound green again, after a busted landing meant we lost test coverage for 7 pushes, and now have multiple failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a145ded68994

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/553fb59b9ca0

Comment 31

[Jonathan Kew [:jfkthame]]

And re-landed, now that the tree's beginning to look greener:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d25ef46858dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a1f5facc2ae

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d25ef46858dc
https://hg.mozilla.org/mozilla-central/rev/0a1f5facc2ae