Last Comment Bug 770538 - [OS.File] Recovering the File from a DirectoryIterator
: [OS.File] Recovering the File from a DirectoryIterator
Status: RESOLVED FIXED
[mentor=Yoric][lang=js]
:
Product: Core
Classification: Components
Component: Networking: File (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla19
Assigned To: Mitesh Pathak [:mr_pathak]
:
Mentors:
Depends on: 808042
Blocks: 770501 OS.File 817267
  Show dependency treegraph
 
Reported: 2012-07-03 08:12 PDT by David Teller [:Yoric] (please use "needinfo")
Modified: 2012-12-01 03:44 PST (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Ist proposed patch for bug 770538 (5.25 KB, patch)
2012-10-10 15:13 PDT, Mitesh Pathak [:mr_pathak]
dteller: feedback+
Details | Diff | Splinter Review
IInd patch for bug 770538 (3.83 KB, patch)
2012-10-11 03:47 PDT, Mitesh Pathak [:mr_pathak]
no flags Details | Diff | Splinter Review
2nd Patch -for bug 770538 (3.84 KB, patch)
2012-10-11 08:03 PDT, Mitesh Pathak [:mr_pathak]
dteller: review+
Details | Diff | Splinter Review
Bug-770538 patch (4.03 KB, patch)
2012-10-11 11:54 PDT, Mitesh Pathak [:mr_pathak]
miteshpathak05: review+
Details | Diff | Splinter Review
2. Getting the Mac version to work (5.44 KB, patch)
2012-10-14 06:56 PDT, David Teller [:Yoric] (please use "needinfo")
nfroyd: review+
Details | Diff | Splinter Review
2. Getting the Mac version to work, v2 (5.55 KB, patch)
2012-10-15 07:51 PDT, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review
2. Getting the Mac version to work, v3 (5.56 KB, patch)
2012-10-23 06:26 PDT, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review
2. Getting the Mac version to work, v3 (5.56 KB, patch)
2012-10-29 12:33 PDT, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review

Description David Teller [:Yoric] (please use "needinfo") 2012-07-03 08:12:21 PDT
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] (please use "needinfo") 2012-07-09 13:41:26 PDT
Not trivial, but this can be an interesting first bug/mentored bug.
Comment 2 jdev005 2012-07-10 11:53:49 PDT
can i be assigned this bug ?
Comment 3 Mayuresh Raut 2012-08-01 12:00:44 PDT
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] 2012-08-10 12:31:28 PDT
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] 2012-08-10 12:32:44 PDT
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] (please use "needinfo") 2012-08-10 12:53:08 PDT
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] (please use "needinfo") 2012-08-12 05:13:42 PDT
Actually, let's keep the Windows version for later. For the time being, only the Unix version is interesting.
Comment 8 Mark Capella [:capella] 2012-08-18 13:12:41 PDT
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] 2012-08-18 23:02:47 PDT
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] (please use "needinfo") 2012-08-19 08:40:17 PDT
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] 2012-10-10 15:13:59 PDT
Created attachment 670153 [details] [diff] [review]
Ist proposed patch for bug 770538
Comment 12 David Teller [:Yoric] (please use "needinfo") 2012-10-11 00:11:24 PDT
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] 2012-10-11 03:47:27 PDT
Created attachment 670334 [details] [diff] [review]
IInd patch for bug 770538

Changes recommended by  David Rajchenbach Teller [:Yoric] applied.
Comment 14 Mitesh Pathak [:mr_pathak] 2012-10-11 03:48:47 PDT
(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] 2012-10-11 08:03:31 PDT
Created attachment 670400 [details] [diff] [review]
2nd Patch -for bug 770538

Changes recommended by  David Rajchenbach Teller [:Yoric] applied.
Comment 16 David Teller [:Yoric] (please use "needinfo") 2012-10-11 10:15:00 PDT
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] 2012-10-11 11:54:38 PDT
Created attachment 670482 [details] [diff] [review]
Bug-770538 patch
Comment 18 David Teller [:Yoric] (please use "needinfo") 2012-10-13 13:15:21 PDT
Try: https://tbpl.mozilla.org/?tree=Try&rev=d53b8198d52b
Comment 19 David Teller [:Yoric] (please use "needinfo") 2012-10-13 23:59:41 PDT
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] 2012-10-14 06:04:48 PDT
(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] (please use "needinfo") 2012-10-14 06:55:54 PDT
Comment on attachment 670482 [details] [diff] [review]
Bug-770538 patch

No need to request my feedback again.
Comment 22 David Teller [:Yoric] (please use "needinfo") 2012-10-14 06:56:45 PDT
Created attachment 671205 [details] [diff] [review]
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] (please use "needinfo") 2012-10-15 07:02:17 PDT
TryServer: https://tbpl.mozilla.org/?tree=Try&rev=09dd7fb8fa3a
Comment 24 Nathan Froyd [:froydnj] 2012-10-15 07:44:30 PDT
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] (please use "needinfo") 2012-10-15 07:50:54 PDT
(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] (please use "needinfo") 2012-10-15 07:51:21 PDT
Created attachment 671432 [details] [diff] [review]
2. Getting the Mac version to work, v2
Comment 27 David Teller [:Yoric] (please use "needinfo") 2012-10-23 06:26:34 PDT
Created attachment 674217 [details] [diff] [review]
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] (please use "needinfo") 2012-10-29 12:33:34 PDT
Created attachment 676264 [details] [diff] [review]
2. Getting the Mac version to work, v3
Comment 30 Ed Morley [:emorley] 2012-10-30 10:58:42 PDT
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) 2012-10-30 12:56:03 PDT
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

Note You need to log in before you can comment on or make changes to this bug.