The default bug view has changed. See this FAQ.

[OS.File] Recovering the File from a DirectoryIterator

RESOLVED FIXED in mozilla19

Status

()

Core
Networking: File
--
enhancement
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Yoric, Assigned: mr_pathak)

Tracking

(Blocks: 1 bug)

unspecified
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=Yoric][lang=js])

Attachments

(2 attachments, 6 obsolete attachments)

Implement a getter |file| that returns the file corresponding to a directory iterator. This can be very useful for bug 770501.
Blocks: 770501
No longer blocks: 770501
Blocks: 770501, 563742
Not trivial, but this can be an interesting first bug/mentored bug.
Whiteboard: [mentor=Yoric][lang=js]

Comment 2

5 years ago
can i be assigned this bug ?

Comment 3

5 years ago
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

5 years ago
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

5 years ago
More concretely, you can find the DirectoryIterator code in these files: http://mxr.mozilla.org/mozilla-central/search?string=directoryiterator
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.
Actually, let's keep the Windows version for later. For the time being, only the Unix version is interesting.
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);
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});
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.
Assignee: nobody → miteshpathak05
(Assignee)

Comment 11

5 years ago
Created attachment 670153 [details] [diff] [review]
Ist proposed patch for bug 770538
Attachment #670153 - Flags: feedback?(dteller)
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.
Attachment #670153 - Flags: feedback?(dteller) → feedback+
(Assignee)

Comment 13

5 years ago
Created attachment 670334 [details] [diff] [review]
IInd patch for bug 770538

Changes recommended by  David Rajchenbach Teller [:Yoric] applied.
Attachment #670153 - Attachment is obsolete: true
Attachment #670334 - Flags: feedback?(dteller)
(Assignee)

Comment 14

5 years ago
(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
(Assignee)

Comment 15

5 years ago
Created attachment 670400 [details] [diff] [review]
2nd Patch -for bug 770538

Changes recommended by  David Rajchenbach Teller [:Yoric] applied.
Attachment #670334 - Attachment is obsolete: true
Attachment #670334 - Flags: feedback?(dteller)
Attachment #670400 - Flags: feedback?(dteller)
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.
Attachment #670400 - Flags: feedback?(dteller) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 670482 [details] [diff] [review]
Bug-770538 patch
Attachment #670482 - Flags: review+
Attachment #670482 - Flags: feedback+
(Assignee)

Updated

5 years ago
Attachment #670482 - Flags: feedback+ → feedback?(dteller)
Try: https://tbpl.mozilla.org/?tree=Try&rev=d53b8198d52b
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.
(Assignee)

Comment 20

5 years ago
(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
Attachment #670400 - Attachment is obsolete: true
Comment on attachment 670482 [details] [diff] [review]
Bug-770538 patch

No need to request my feedback again.
Attachment #670482 - Flags: feedback?(dteller)
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?
Attachment #671205 - Flags: review?(nfroyd)
TryServer: https://tbpl.mozilla.org/?tree=Try&rev=09dd7fb8fa3a
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?
Attachment #671205 - Flags: review?(nfroyd) → review+
(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.
Created attachment 671432 [details] [diff] [review]
2. Getting the Mac version to work, v2
Attachment #671205 - Attachment is obsolete: true
Attachment #671432 - Flags: review+
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
Attachment #671432 - Attachment is obsolete: true
Attachment #674217 - Flags: review+
Created attachment 676264 [details] [diff] [review]
2. Getting the Mac version to work, v3
Attachment #674217 - Attachment is obsolete: true
Attachment #676264 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea6edd3749ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/354250ed1e72
Flags: in-testsuite+
Keywords: checkin-needed
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
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
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/d25ef46858dc
https://hg.mozilla.org/mozilla-central/rev/0a1f5facc2ae
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 808042

Updated

4 years ago
Blocks: 817267
You need to log in before you can comment on or make changes to this bug.