Closed Bug 921229 Opened 11 years ago Closed 11 years ago

[OS.File] Functions that remove a file/directory should accept an option |force| that overrides read-only when possible

Categories

(Toolkit Graveyard :: OS.File, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla28

People

(Reporter: mossop, Assigned: fhd)

References

Details

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

Attachments

(1 file, 7 obsolete files)

Sometimes you need to be able to set a file's permissions. nsIFile offers this but OS.File does not.
Will a Unix-only version be sufficient for you?
Windows-style permissions are heckishly more complex than Unix permissions, so I'd rather we not implement them until we have a specific scenario.
Unix is the main requirement, for executableness in particular. One other reason we've used this in the past is to force a file to be writable so that we're able to delete it, I don't know if that is actually a problem on Windows or if OS.File.delete would be able to delete such files anyway.
Dave, I probably won't have time to work on that bug in the near future, but I'm willing to mentor someone. Are you interested in writing the patch or do you have someone who would be?
Whiteboard: [mentor=Yoric][lang=js][Async]
I've been looking into this and the UNIX part is done, it's fairly straightforward. I went for a OS.File.chmod function, but I'm wondering if it wouldn't be nicer to have a permissions property the user can set, that's how nsIFile does it.

The remaining question is: What to do about Windows? nsILocalFile has some crude mapping of permissions:

- If anyone can read, all can read
- If anyone can write, all can write
- Executing rights are ignored

From what little I know about Windows file permissions, we should be able to do better than that. But if it's not really needed, we might as well not support permission changing on Windows to begin with. Thoughts?
I believe that designing a correct API is the most difficult part of this bug.

What about the following?

/**
 * @param {object=} permissions An object that may contain the following
 * fields: 
 * - {object} unix An object that may contain the following fields:
 *    {object} user: The permissions granted to the user, as an object containing boolean fields |read|, |write|, |execute|. Fields that do not appear default to |false|.
 *    {string} group: as above
 *    {string} others: as above
 *
 */
OS.File.setPermissions(path, permissions)

... and add support for Windows later, as a field |win|.

Felix, Nathan, what do you think?
Flags: needinfo?(nfroyd)
Flags: needinfo?(fhd)
I generally like it David, some remarks:

1. Shouldn't we try to abstract permissions away instead of requiring developers to have platform-specific code? How about having cross-platform permissions (basically the intersection of what's possible) in addition to "unix" and "win"?

2. I'm not completely happy with the name setPermission, I try to reserve "set" for property setters typically. But maybe that's just me :) However, I wouldn't want something with a different name here, since it's essentially the same function.

I've finished this and added some tests, so apart from the design decisions, it's ready for review I guess. I'll attach what I have in a bit.
Flags: needinfo?(fhd)
Attached patch Added OS.File.chmod for UNIX (obsolete) — Splinter Review
I've attached a patch for a simple implementation that uses "chmod" functions and an octal UNIX-style permission mask. We're in the middle of discussing the API, but I still thought it might be a good starting point to have some actual code here.

I've added both a function and a method, so there are two ways to do this:

OS.File.chmod("/that/file", 01337)

and

OS.File.open("/that/file").then(function(file) file.chmod(01337));

Note that octal literals are apparently deprecated, and also quite error-prone, so they're not a great option. It's what nsIFile uses though.
Attachment #813993 - Flags: review?(dteller)
Attachment #813993 - Flags: checkin+
Attachment #813993 - Flags: checkin+
(In reply to David Rajchenbach Teller [:Yoric] <on summit, unavailable until October 8th> from comment #5)
> /**
>  * @param {object=} permissions An object that may contain the following
>  * fields: 
>  * - {object} unix An object that may contain the following fields:
>  *    {object} user: The permissions granted to the user, as an object
> containing boolean fields |read|, |write|, |execute|. Fields that do not
> appear default to |false|.
>  *    {string} group: as above
>  *    {string} others: as above
>  *
>  */
> OS.File.setPermissions(path, permissions)
> 
> ... and add support for Windows later, as a field |win|.
> 
> Felix, Nathan, what do you think?

Seems a little heavyweight, but we could do that.  You'd probably want to make the defaults to be user: read/write, at least, even if group and other didn't have any permissions.  Actually, you should probably just require people to always provide |user|, |group|, and |other| just to avoid unpleasant mistakes.

Alternately, we could define OS.File.{R,RW,RX,RWX} constants and define those to be the acceptable values for the |user|, |group|, and |other| fields.

I tend to think that we don't need to support any Windows-style permissions.
Flags: needinfo?(nfroyd)
Assignee: nobody → fhd
(In reply to Nathan Froyd (:froydnj) from comment #8)
> Seems a little heavyweight, but we could do that.

I agree.  Given comment 2, I wonder if a better approach would be to:

* Add an optional "force" param to functions that delete a file.

* Add a method to set a file executable flag - and it would just be a noop on Windows.

That lets us avoid a heavy API that isn't cross-platform and for which there isn't a current use-case.  When a real use-case can be articulated, we can implement that and do whatever makes sense for Windows within the constraints of that use-case.
(In reply to Mark Hammond (:markh) from comment #9)
> (In reply to Nathan Froyd (:froydnj) from comment #8)
> > Seems a little heavyweight, but we could do that.
> 
> I agree.  Given comment 2, I wonder if a better approach would be to:
> 
> * Add an optional "force" param to functions that delete a file.

This is actually the main need I have left for this. Not having to do a chmod before removing would be just fine.

> * Add a method to set a file executable flag - and it would just be a noop
> on Windows.

I think I no longer need chmod for this as OS.File.open allows me to set the permissions through there.
In that case, I believe that we should repurpose this bug.
Summary: Add OS.File.chmod() → [OS.File] Functions that remove a file/directory should accept an option |force| that overrides read-only when possible
Comment on attachment 813993 [details] [diff] [review]
Added OS.File.chmod for UNIX

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

Canceling the review since we have repurposed the bug.
However, I'll give some feedback.

::: toolkit/components/osfile/modules/osfile_async_worker.js
@@ +350,5 @@
>         });
>     },
> +   File_prototype_chmod: function chmod(fd, permissions) {
> +     return withFile(fd,
> +       function do_stat() {

Nit: wrong closure name.

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +200,5 @@
> +      * @throws {OS.File.Error} If the permissions could not be changed.
> +      */
> +     File.prototype.chmod = function chmod(mask) {
> +       throw new File.Error("chmod is currently not supported on Windows");
> +     };

Side-note: in that case, this was a bit early for r?

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +955,5 @@
> +      yield file.chmod(permissions2);
> +      let info2 = yield file.stat();
> +      test.is(info2.unixMode, permissions2, "Permissions have been changed");
> +    } finally {
> +      yield file.close();

I'm not sure this would have worked under Windows, due to the restrictions on touching a file while it's already opened.
Attachment #813993 - Flags: review?(dteller)
Sounds good to me, if that's the only remaining use case. Something like this then?

/**
 * Remove an existing file.
 *
 * @param {string} path The name of the file.
 * @param {bool} force If |true|, remove the file even when lacking write
 * permission.
 */
File.remove = function remove(path, force) {
More like

/**
 * Remove an existing file.
 *
 * @param {string} path The name of the file.
 * @param {object=} options Optionally, an object that may contain the following fields:
 *  - {boolean} ignorePermissions If |true|, remove the file even when lacking write
 *     permission.
 */
File.remove = function remove(path, options)
ignorePermissions is more precise, force would be in line to POSIX rm though: It only ignores permissions and doesn't print errors. But agreed, let's keep it clear. I'll work on that in a bit.
Note that I need this for OS.File.removeEmptyDir too, unless you want to make OS.File.remove work for directories too.
No, you are right, we need this for OS.File.removeEmptyDir and also for bug 923540.
Blocks: 923540
I just tested this, and both OS.File.remove and OS.File.removeEmptyDir already ignore write permissions when removing. At least on UNIX, where they're using unlink and rmdir under the hood, which ignore write permissions anyway.

Can you help me reproduce the issue you're having, Dave?
(In reply to Felix H. Dahlke from comment #18)
> I just tested this, and both OS.File.remove and OS.File.removeEmptyDir
> already ignore write permissions when removing. At least on UNIX, where
> they're using unlink and rmdir under the hood, which ignore write
> permissions anyway.
> 
> Can you help me reproduce the issue you're having, Dave?

This is a long standing issue in code that I'm converting from nsIFile APIs. So if OS.File APIs already do this then I guess there is nothing to do here
(In reply to Felix H. Dahlke from comment #18)
> I just tested this, and both OS.File.remove and OS.File.removeEmptyDir
> already ignore write permissions when removing. At least on UNIX, where
> they're using unlink and rmdir under the hood, which ignore write
> permissions anyway.

Did you test on Windows?  It sounds like from the above you might have only tested on Linux/OSX.
Flags: needinfo?(fhd)
Good point. 
* Under Unix, OS.File.remove() will fail if the directory is not write-accessible but not if the file is not write-accessible.
* Under Windows, OS.File.remove() will fail with ERROR_ACCESS_DENIED if the file is not write-accessible.

So, we probably need to fix the Windows behavior. Dave, do you need to alter the Unix behavior?
(In reply to David Rajchenbach Teller [:Yoric] from comment #21)
> Good point. 
> * Under Unix, OS.File.remove() will fail if the directory is not
> write-accessible but not if the file is not write-accessible.

Did you mean OS.File.removeEmptyDir() here or are you talking about files? We should be able to remove regardless of file access permissions.
(In reply to Nathan Froyd (:froydnj) from comment #20)
> Did you test on Windows?  It sounds like from the above you might have only
> tested on Linux/OSX.

Now, only tested the UNIX code path. But according to David's test, it doesn't work on Windows.

(In reply to Dave Townsend (:Mossop) from comment #22) 
> Did you mean OS.File.removeEmptyDir() here or are you talking about files?
> We should be able to remove regardless of file access permissions.

I think what he meant was that a file cannot be deleted if it is inside a directory the user doesn't have write permission on, which is in line with rm -f, it cannot be deleted without changing the permissions of the containing directory.

According to my tests (on UNIX), both remove and removeEmptyDir work fine with a file/empty dir lacking write permission. It's the containing dir's permissions that really matter.
Flags: needinfo?(fhd)
Anyway, looks like a Windows-only issue then. I can set up a Windows build environment and look into it.

I'd say we don't add a force option for this, it should just try a bit harder to remove a file/directory on Windows. Dave, Nathan, does that sound good to you?
Forgot the needinfo flags.
Flags: needinfo?(dtownsend+bugmail)
Flags: needinfo?(dteller)
(In reply to Felix H. Dahlke from comment #24)
> Anyway, looks like a Windows-only issue then. I can set up a Windows build
> environment and look into it.
> 
> I'd say we don't add a force option for this, it should just try a bit
> harder to remove a file/directory on Windows. Dave, Nathan, does that sound
> good to you?

Sounds fine to me
Flags: needinfo?(dtownsend+bugmail)
Sounds fine to me, too.
Flags: needinfo?(dteller)
Attached patch bug-921229.patch (obsolete) — Splinter Review
This patch should do, OS.File.remove is now clearing the read-only flag if set. This wasn't necessary for removeEmptyDir, in case you're wondering. And since removeDir is implemented in terms of remove and removeEmptyDir, that works as well.

I'm still planning to add a test for this, so I just requested feedback for now.
Attachment #813993 - Attachment is obsolete: true
Attachment #820049 - Flags: feedback?(dteller)
Comment on attachment 820049 [details] [diff] [review]
bug-921229.patch

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

::: toolkit/components/osfile/modules/osfile_win_back.jsm
@@ +338,5 @@
> +        SysFile.SetFileAttributes =
> +          declareFFI("SetFileAttributesW", ctypes.winapi_abi,
> +                     /*return*/         Type.zero_or_nothing,
> +                     /*fileName*/       Type.path,
> +                     /*fileAttributes*/ Type.DWORD);

That looks good, but take a look at the changes in bug 801376, you will need to change the declareFFI for performance/memory use reasons.

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +339,5 @@
>        *     not exist. |true| by default.
>        *
>        * @throws {OS.File.Error} In case of I/O error.
>        */
>       File.remove = function remove(path, options = {}) {

In most cases, we will succeed at removing without having to tweak attributes, so don't do the {Get, Set}FileAttribute stuff until it has failed.

@@ +342,5 @@
>        */
>       File.remove = function remove(path, options = {}) {
> +       let attributes = WinFile.GetFileAttributes(path);
> +       if (attributes == Const.INVALID_FILE_ATTRIBUTES) {
> +         if ("ignoreAbsent" in options && !options.ignoreAbsent) {

That looks fishy. Why should ignoreAbsent change our behavior when that function fails?
Attachment #820049 - Flags: feedback?(dteller) → feedback+
(In reply to David Rajchenbach Teller [:Yoric] from comment #29)
> Comment on attachment 820049 [details] [diff] [review]
> bug-921229.patch
> 
> Review of attachment 820049 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/modules/osfile_win_back.jsm
> @@ +338,5 @@
> > +        SysFile.SetFileAttributes =
> > +          declareFFI("SetFileAttributesW", ctypes.winapi_abi,
> > +                     /*return*/         Type.zero_or_nothing,
> > +                     /*fileName*/       Type.path,
> > +                     /*fileAttributes*/ Type.DWORD);
> 
> That looks good, but take a look at the changes in bug 801376, you will need
> to change the declareFFI for performance/memory use reasons.

That hasn't landed in central yet, has it? Also, seems like this should be used for other FFIs of Win32 system calls as well.

> ::: toolkit/components/osfile/modules/osfile_win_front.jsm
> @@ +339,5 @@
> >        *     not exist. |true| by default.
> >        *
> >        * @throws {OS.File.Error} In case of I/O error.
> >        */
> >       File.remove = function remove(path, options = {}) {
> 
> In most cases, we will succeed at removing without having to tweak
> attributes, so don't do the {Get, Set}FileAttribute stuff until it has
> failed.

I went with that to keep the code simple, but I guess you're right, read-only files are probably a corner case.

> @@ +342,5 @@
> >        */
> >       File.remove = function remove(path, options = {}) {
> > +       let attributes = WinFile.GetFileAttributes(path);
> > +       if (attributes == Const.INVALID_FILE_ATTRIBUTES) {
> > +         if ("ignoreAbsent" in options && !options.ignoreAbsent) {
> 
> That looks fishy. Why should ignoreAbsent change our behavior when that
> function fails?

Yes, I should have checked for winLastError.
Attached patch bug-921229.patch (obsolete) — Splinter Review
Here's a patch that tries to delete the file first, only looking for the read-only flag if that fails. Unfortunately, this makes the code more complex, let me know if you have any ideas for making it cleaner.
Attachment #820049 - Attachment is obsolete: true
Attachment #820995 - Flags: feedback?(dteller)
Comment on attachment 820995 [details] [diff] [review]
bug-921229.patch

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

Looks good to me. Next part is, of course, testing.
Side-note: In the future, could you number your patches? Also, giving them a human-readable name won't hurt :)
Attachment #820995 - Flags: feedback?(dteller) → feedback+
Added a test. I wasn't really sure where to put it, that place seemed to make the most sense. I noticed that a bunch of FFIs are tested in worker_test_osfile_win.js/worker_test_osfile_unix.js. I'm not sure if I should add a test for GetFileAttributes/SetFileAttributes there, since we already have this covered with the test I added.
Attachment #820995 - Attachment is obsolete: true
Attachment #822035 - Flags: review?(dteller)
Comment on attachment 822035 [details] [diff] [review]
Remove files with the read-only flag set

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

r=me with the change below

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +845,5 @@
>    });
>    ok(!exn, "test_remove_file: ignoreAbsent works");
> +
> +  if (OS.Win) {
> +    let file_name = "test_osfile_front_file.tmp";

Could you just give it a name that matches the specific feature you are testing?

@@ +854,5 @@
> +                                  OS.Constants.Win.FILE_ATTRIBUTE_READONLY);
> +    OS.File.remove(file_name);
> +    ok(!OS.File.exists(file_name),
> +       "test_remove_file: test file has been removed");
> +  }

Ok, that's one of the only tests that we should effectively put in this file :)
Attachment #822035 - Flags: review?(dteller) → review+
OK, fixed.
Attachment #822035 - Attachment is obsolete: true
Attachment #822338 - Flags: review?(dteller)
Comment on attachment 822338 [details] [diff] [review]
Remove files with the read-only flag set (2)

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

Looks good, thanks.
r=me, assuming that it passes tests.
Also, don't forget to change your commit message so that it starts with "Bug 921229" and ends with ";r=yoric"
Attachment #822338 - Flags: review?(dteller) → review+
Right, forgot that. Also set the checkin flag, that's how it's supposed to be, right?
Attachment #822338 - Attachment is obsolete: true
Attachment #822356 - Flags: review?(dteller)
Attachment #822356 - Flags: checkin+
Attachment #822356 - Flags: review?(dteller) → review+
Please can you rebase the patch? :-)
Also can you confirm the tests pass?

[/c/src-gecko/inbound]$ qimp 921229
Fetching... done
Parsing... done
adding 921229 to series file
renamed 921229 -> bug-921229.patch
applying bug-921229.patch
patching file toolkit/components/osfile/modules/osfile_win_back.jsm
Hunk #1 FAILED at 323
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/osfile/modules/osfile_win_back.jsm.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug-921229.patch
Keywords: checkin-needed
Rebased my patch. Since the declareLazyFFI stuff has landed in the meantime, I'm using that now.
Attachment #822356 - Attachment is obsolete: true
Attachment #823848 - Flags: review?(emorley)
Attachment #823848 - Flags: review?(dteller)
And here's a patch without the weird indentation, sorry about that...
Attachment #823848 - Attachment is obsolete: true
Attachment #823848 - Flags: review?(emorley)
Attachment #823848 - Flags: review?(dteller)
Attachment #823852 - Flags: review?(emorley)
Attachment #823852 - Flags: review?(dteller)
Comment on attachment 823852 [details] [diff] [review]
Remove files with the read-only flag set (5)

Thank you :-)

(I'm not a module owner/peer of this component, so I'll leave the review to David).
Attachment #823852 - Flags: review?(emorley)
Attachment #823852 - Flags: review?(dteller) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/66b33f3f3594
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [mentor=Yoric][lang=js][Async] → [mentor=Yoric][lang=js][Async][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/66b33f3f3594
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=Yoric][lang=js][Async][fixed-in-fx-team] → [mentor=Yoric][lang=js][Async]
Target Milestone: --- → mozilla28
Blocks: 935189
Depends on: 1211243
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: