924874 - [OS.File] Provide alternative to nsIFile.diskSpaceAvailable

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, enhancement)

Description

[Nils Maier [:nmaier]]

http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIFile.idl#398

Right now, there does not seem to be any way to query the free diskspace with OS.File.

Comment 1

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

Indeed, there is no way at the moment.
I don't have time to add this right now, but if you are interested in working on that bug, I'm quite willing to mentor/review.

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: OSFile_FreeSpace.patch

I've added the OS.File.getAvailableFreeSpace function which allows to query the amount of free space, in bytes, for the current user. It works on Windows, but I'm not sure about the Linux/Unix part, since I don't have access to a Linux machine at the moment.

Comment 3

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

Thanks.
I'll try and look at it this week, but I'm currently quite busy, so I make no promise.
You'll find the results of the tests here: https://tbpl.mozilla.org/?tree=Try&rev=f9854b1a3697

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: OSFile_FreeSpace.patch

Thank you for uploading to the try server. Based on the results I was able to fix the Linux part. It should be correctly working with this patch.

Comment 5

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

Comment on attachment 8361075 [details] [diff] [review]
OSFile_FreeSpace.patch

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

Looks good.
I'm not adding r+ yet, because I want tests.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +723,5 @@
> + * @return {number} The number of bytes available for the current user.
> + * @throws {OS.File.Error} In case of any error.
> + */
> +File.getAvailableFreeSpace = function getAvailableFreeSpace(sourcePath) {
> +  return Scheduler.post("getAvailableFreeSpace", 

Nit: Please remove that trailing whitespace.

::: toolkit/components/osfile/modules/osfile_unix_back.jsm
@@ +217,5 @@
>           Type.timevals = new SharedAll.Type("two timevals",
>             ctypes.ArrayType(Type.timeval.implementation, 2));
>         }
>  
> +       

Nit: Unnecessary whitespace.

@@ +220,5 @@
>  
> +       
> +       // Type fsblkcnt_t, used by structure |STATVFS|
> +       Type.fsblkcnt_t =
> +         Type.unsigned_long.withName("fsblkcnt_t");

Are you sure it's an unsigned long on all platforms?

@@ +221,5 @@
> +       
> +       // Type fsblkcnt_t, used by structure |STATVFS|
> +       Type.fsblkcnt_t =
> +         Type.unsigned_long.withName("fsblkcnt_t");
> +         

Nit: Unncessary whitespace.

@@ +223,5 @@
> +       Type.fsblkcnt_t =
> +         Type.unsigned_long.withName("fsblkcnt_t");
> +         
> +       // Structure |STATVFS|
> +       Type.STATVFS =

Why uppercase? It's lowercase in Posix, so let's keep it lowercase here.

@@ +238,5 @@
> +            { f_fsid:    Type.unsigned_long.implementation },
> +            { f_flag:    Type.unsigned_long.implementation },
> +            { f_namemax: Type.unsigned_long.implementation }
> +           ]));
> +       

Nit: Whitespace.

@@ +495,5 @@
>  
> +       declareLazyFFI(SysFile,  "statvfs", libc, "statvfs", ctypes.default_abi,
> +                    /*return*/ Type.negativeone_or_nothing,
> +                    /*path*/   Type.path,
> +                    /*off_in*/ Type.STATVFS.out_ptr);                    

According to the manpage, this shouldn't be called off_in but buf.
Nit: Whitespace.

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +362,5 @@
> +      */
> +     File.getAvailableFreeSpace = function Unix_getAvailableFreeSpace(sourcePath) {
> +       let fileSystemInfo = new Type.STATVFS.implementation();
> +       let fileSystemInfoPtr = fileSystemInfo.address();
> +       

Nit: whitespace (hint: look at the review link, they'll be clearly visible).

::: toolkit/components/osfile/modules/osfile_win_back.jsm
@@ +280,5 @@
> +                    /*directoryName*/ Type.path,
> +                    /*freeBytesForUser*/ Type.uint64_t.out_ptr,
> +                    /*totalBytesForUser*/ Type.uint64_t.out_ptr,
> +                    /*freeTotalBytesOnDrive*/ Type.uint64_t.out_ptr);                   
> +                   

Nit: whitespace.

Comment 6

[Alessio Placitelli [:Dexter]]

Thank you for the time you spend reviewing my submission! I'll submit an updated patch with proper tests later this evening.

I just have one (probably naive) question: how can I check if fsblkcnt_t is not unsigned long on other platforms?

Comment 7

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

(In reply to Alessio Placitelli from comment #6)
> Thank you for the time you spend reviewing my submission! I'll submit an
> updated patch with proper tests later this evening.
> 
> I just have one (probably naive) question: how can I check if fsblkcnt_t is
> not unsigned long on other platforms?

Good question. We don't have a good mechanism for this, so I'd just check the documentation of Linux and MacOS X, and some other BSD.

Comment 8

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

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #7)
> (In reply to Alessio Placitelli from comment #6)
> > I just have one (probably naive) question: how can I check if fsblkcnt_t is
> > not unsigned long on other platforms?
> 
> Good question. We don't have a good mechanism for this, so I'd just check
> the documentation of Linux and MacOS X, and some other BSD.

Actually, I am misleading you. You should look at how we define mode_t in OS.File:
- native code is here: http://dxr.mozilla.org/mozilla-central/source/dom/system/OSFileConstants.cpp?from=OSFileConstants.cpp#517
- JS code is here: http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_unix_back.jsm?from=osfile_unix_back.jsm#88

Comment 9

[Alessio Placitelli [:Dexter]]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #8)
> (In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from
> comment #7)
> > (In reply to Alessio Placitelli from comment #6)
> > > I just have one (probably naive) question: how can I check if fsblkcnt_t is
> > > not unsigned long on other platforms?
> > 
> > Good question. We don't have a good mechanism for this, so I'd just check
> > the documentation of Linux and MacOS X, and some other BSD.
> 
> Actually, I am misleading you. You should look at how we define mode_t in
> OS.File:
> - native code is here:
> http://dxr.mozilla.org/mozilla-central/source/dom/system/OSFileConstants.
> cpp?from=OSFileConstants.cpp#517
> - JS code is here:
> http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/
> modules/osfile_unix_back.jsm?from=osfile_unix_back.jsm#88

Thanks, this comment saved the day! I'll complete the patch and send it back ASAP.

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: OSFile_FreeSpace.patch

I've also provided an xpcshell test which checks for the available space function to return a value greater than 0.

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: OSFile_FreeSpace.patch

Comment 12

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

Comment on attachment 8365550 [details] [diff] [review]
OSFile_FreeSpace.patch

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

Looks good to me, thanks for the patch.
Try: https://tbpl.mozilla.org/?tree=Try&rev=70231f3e9925

::: toolkit/components/osfile/tests/xpcshell/test_available_free_space.js
@@ +33,5 @@
> +  // Query for available bytes for user
> +  let availableBytes = yield OS.File.getAvailableFreeSpace(dir);
> +
> +  do_check_true(!!availableBytes);
> +  do_check_true(availableBytes > 0);

Given that tests are executed currently, we probably can't do better. It's sad.

Comment 13

[Alessio Placitelli [:Dexter]]

Thank you for your time reviewing the patch: I've just noticed that on the Tryserver tests fail on Linux Debug and Linux x64 Debug. Also the build is busted on some B2G builds, but I can't seem to get a hang on what's wrong. Any clue?

Comment 14

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

For the B2G build, I suspect that it's not your fault.
For the other builds, I assume that some value doesn't have the right size.

Here are a few suggestions:
- since you're really using only f_bfree and f_bsize, let's make the struct a HollowStruct (see |struct dirent|, for example);
- we actually have a reference implementation of checking out free disk space with nsIFile.diskSpaceAvailable, you should use both of them and check that they have approximately the same result (I say approximately because stuff may happen while we're running the tests).

Comment 15

[Alessio Placitelli [:Dexter]]

That's weird. I've compiled a Linux debug build locally and everything went smoothly.

Comment 16

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

(In reply to Alessio Placitelli from comment #15)
> That's weird. I've compiled a Linux debug build locally and everything went
> smoothly.

No better idea, sorry.

Comment 17

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

Any progress?

Comment 18

[Alessio Placitelli [:Dexter]]

Attachment: OSFile_FreeSpace.patch

This patch uses hollow structures and fixes the tests problems I had with the previous patch. It took me a little bit to figure out how to fix the B2G and Android builds, sorry for the delay!

I've uploaded it to the tryserv and the results are available here: https://tbpl.mozilla.org/?tree=Try&rev=d191223db9f6

Comment 19

[Alessio Placitelli [:Dexter]]

Attachment: OSFile_FreeSpace.patch

Comment 20

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

Comment on attachment 8372984 [details] [diff] [review]
OSFile_FreeSpace.patch

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

Looks good, thank you for the patch.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +786,5 @@
>  
>  /**
> + * Gets the number of bytes available on disk to the current user.
> + *
> + * @param {string} sourcePath The platform-specific path on the disk to query

« Platform-specific path to a directory on the disk to query for free available bytes. »

Comment 21

[Alessio Placitelli [:Dexter]]

Attachment: OSFile_FreeSpace.patch

Awesome, thank you for your time reviewing!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/758535fb8b0f

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/758535fb8b0f