919771 - [OS.File] Preserve permissions when moving/copying files between volumes

Status: RESOLVED WONTFIX

(Toolkit Graveyard :: OS.File, defect)

Description

[Marco Castelluccio [:marco]]

In bug 749826 I'd need to move a directory (and its contents) from the temporary directory.
I need to preserve permissions even if I'm moving files from a volume to another (on Linux /tmp is usually on another volume).

Comment 1

[Marco Castelluccio [:marco]]

Attachment: moveDirectory.js

This is the function I've written to move the directory, I wonder if we can add it to OS.File so that we don't need to re-implement it every time we need it.

Comment 2

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

(In reply to Marco Castelluccio [:marco] from comment #1)
> Created attachment 808844 [details]
> moveDirectory.js
> 
> This is the function I've written to move the directory, I wonder if we can
> add it to OS.File so that we don't need to re-implement it every time we
> need it.

Well, that won't work if the directory contains subdirectories.

Comment 3

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

Under Unix, to preserve permissions of a single file:
- if the move stays on the same volume, permissions are already preserved;
- if the move crosses volume boundaries, we actually copy the file and lose permission – we will need to add a call to stat() and a call to chmod().

Under Windows, to preserve permissions of a single file:
- if the move stays on the same volume, permissions are already preserved;
- if the move crosses volume boundaries, Windows itself actually copies the file and loses permissions – we will need to replace our call to MoveFile() by a call to MoveFileEx() to instruct Windows to not do this, then call CopyFile(), then add a call to GetFileSecurity() and a call to SetFileSecurity().

I would like to avoid doing this for directories just yet.

Comment 4

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

Marco, I don't have time to work on this right now, but if you wish to pick up the bug, I'll be glad to mentor you along.

Comment 5

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

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> Well, that won't work if the directory contains subdirectories.

Sorry, I misread your patch. Let's handle this in another bug, though.

Comment 6

[Marco Castelluccio [:marco]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> Marco, I don't have time to work on this right now, but if you wish to pick
> up the bug, I'll be glad to mentor you along.

Ok, sure, I'd need also other things in OS.File so it'd be good to start gaining some knowledge about its internals.

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Patch

I'm not sure we need the Windows counterpart, do we?

Comment 8

[Marco Castelluccio [:marco]]

Attachment: Patch

I'm not sure how to test this.

Comment 9

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

Comment on attachment 813619 [details] [diff] [review]
Patch

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

If you only implement it for Unix, please call this unixPreservePermissions.
Unfortunately, I don't think we can test it yet on Try. You'll have to do this locally.

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +542,5 @@
>             source = File.open(sourcePath);
> +
> +           let openOptions = {};
> +           if (options.preservePermissions) {
> +             let info = File.stat(sourcePath);

Calling |fstat| should be faster.

@@ +550,2 @@
>             if (options.noOverwrite) {
> +             dest = File.open(destPath, {create:true}, openOptions);

Could you take the opportunity to add a whitespace after ":"?

@@ +553,5 @@
> +             if (options.preservePermissions) {
> +               File.remove(destPath);
> +             }
> +
> +             dest = File.open(destPath, {trunc:true}, openOptions);

With |trunc: true|, I'm almost sure that |openOptions| will be ignored. Therefore, if preservePermissions is set, you should call File.remove(destPath, {ignoreAbsent: true}) and then |dest = File.open(destPath, {create: true}, openOptions)|.

Comment 10

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

By the way, under Linux (only), we can test on Try, by moving the file to /dev/shm.

Comment 11

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

Under MacOS, we can use the trick shown here: http://www.frederico-araujo.com/2008/12/18/blazing-fast-firefox-using-osx-ramdisk/ (thanks dustin)

Comment 12

[Marco Castelluccio [:marco]]

I've managed to write a test for Linux, I'm not sure how to use that trick for Mac OS X.

What should we do for copy and move in the case of single volume when |unixPreservePermissions=false|?
At the moment we're preserving the permissions in the case of move, not preserving in the case of copy.

Comment 13

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

(In reply to Marco Castelluccio [:marco] from comment #12)
> What should we do for copy and move in the case of single volume when
> |unixPreservePermissions=false|?
> At the moment we're preserving the permissions in the case of move, not
> preserving in the case of copy.

In that case, we do whatever is simplest.

Comment 14

[Marco Castelluccio [:marco]]

Attachment: Patch v2

The tests are working on Linux, I'm not sure how to create automated tests for Mac.

Comment 15

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

Comment on attachment 815169 [details] [diff] [review]
Patch v2

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

Could you please apply the suggestions I made in my previous feedback?

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +545,5 @@
> +           if (options.preservePermissions) {
> +             let statData = new OS.Shared.Type.stat.implementation();
> +             let statDataPtr = statData.address();
> +             throw_on_negative("stat", UnixFile.fstat(source.fd, statDataPtr));
> +             openOptions.unixMode = exports.OS.Shared.Type.mode_t.importFromC(statData.st_mode & MODE_MASK);

I meant |source.stat()| (which uses |fstat|), but that should work, too.

Comment 16

[Marco Castelluccio [:marco]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #15)
> Comment on attachment 815169 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 815169 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could you please apply the suggestions I made in my previous feedback?

Ah, I forgot to change the name!
I didn't apply the other suggestion because looks like the options aren't ignored with |trunc:true|, http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_unix_front.jsm#219
Let me know if you want me to modify that anyways.

Comment 17

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

(In reply to Marco Castelluccio [:marco] from comment #16)
> Ah, I forgot to change the name!
> I didn't apply the other suggestion because looks like the options aren't
> ignored with |trunc:true|,
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/
> modules/osfile_unix_front.jsm#219
> Let me know if you want me to modify that anyways.

I'm pretty sure that they are ignored by the lower-level C API.

(In reply to Marco Castelluccio [:marco] from comment #12)
> I've managed to write a test for Linux, I'm not sure how to use that trick
> for Mac OS X.

You'll need to write a script and execute it with nsIProcess.

Comment 18

[Marco Castelluccio [:marco]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> (In reply to Marco Castelluccio [:marco] from comment #16)
> > Ah, I forgot to change the name!
> > I didn't apply the other suggestion because looks like the options aren't
> > ignored with |trunc:true|,
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/
> > modules/osfile_unix_front.jsm#219
> > Let me know if you want me to modify that anyways.
> 
> I'm pretty sure that they are ignored by the lower-level C API.
> 
> (In reply to Marco Castelluccio [:marco] from comment #12)
> > I've managed to write a test for Linux, I'm not sure how to use that trick
> > for Mac OS X.
> 
> You'll need to write a script and execute it with nsIProcess.

Will that work on our try servers? Doesn't something like that need admin privileges on Mac?
Anyway I don't own a Mac, so I'll get back to this bug if I manage to build a Mac virtual machine.

Comment 19

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

It is my understanding that this script doesn't require admin privileges.

Comment 20

[Marco Castelluccio [:marco]]

Attachment: Patch v3

Implemented tests for Mac.
The try run is green: https://tbpl.mozilla.org/?tree=Try&rev=ece15e2f014e

Comment 21

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

Comment on attachment 8375611 [details] [diff] [review]
Patch v3

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

The core patch looks good, for Unix. Don't you need a Windows version, too?

Also, I'd prefer if you could make the test a xpcshell-test, this makes it easier to launch independently.

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +624,5 @@
> +             if (options.unixPreservePermissions) {
> +               File.remove(destPath);
> +             }
> +
> +             dest = File.open(destPath, {trunc: true, append: false}, openOptions);

If you remove the file, you don't need |trunc|, so you can merge the two |dest = File.open(...)|.

::: toolkit/components/osfile/tests/mochi/create_volume.sh
@@ +1,5 @@
> +#!/bin/bash
> +
> +NumSectors=$((2*1024))
> +
> +DeviceName=`hdid -nomount ram://$NumSectors`

Ok, that should work on MacOS X. Do you have ideas on how to extend this to other OSes?

Comment 22

[Marco Castelluccio [:marco]]

I was thinking that for my use case it's probably better to explicitly change the permissions than copying the permissions of the source file (it's better not to rely on the assumption that the source file will always have the right permissions).

Can we add a function to OS.File to change file permissions?

Comment 23

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

> Can we add a function to OS.File to change file permissions?

Fine with me.

I guess it should look like:
OS.File.setPermissions(path, {unixMode: ..., winSecurity: ...})

Do you know how you intend this to work on Windows?

Comment 24

[Barret Rennie [:barret] (they/them)]

OSFIle is being replaced with IOUtils and PathUtils.