53474 - nsLocalFile::CopyTo not fully implemented on Unix [FIX]{REVIEW]

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Conrad Carlen (not reading bugmail)]

In nsLocalFile::CopyTo in the file nsLocalFileUnix.cpp, it returns 
NS_ERROR_NOT_IMPLEMENTED if it is a directory. This method works on Mac and Win.

Comment 1

[Christopher Blizzard (:blizzard)]

-> [e-mail]

Comment 2

[Christopher Blizzard (:blizzard)]

brendan, did you fix this?  I thought you did.

Comment 3

[Brendan Eich [:brendan]]

No, I didn't fix it.  Ok, I'll hack on it.

/be

Comment 4

[Brendan Eich [:brendan]]

Slip.

Comment 5

[Brendan Eich [:brendan]]

My laptop is dying (bad SIMM?), I'm doomed for 0.9.1 unless a patch is in hand.

/be

Comment 6

[Pete Collins (MDG)]

I beleive i have the fix for this bug here.

http://bugzilla.mozilla.org/show_bug.cgi?id=70722

--pete

Comment 7

[Pete Collins (MDG)]

Attachment: patch implementation for copying dirs on unix

Comment 8

[Pete Collins (MDG)]

Can i get some review love so we can close this bug?

Thanks

--pete

Comment 9

[timeless]

1. please fix whitespace.
2. you use |if(| and |if (| i think the code prefers the latter, i know i do.
3. please split things like |if (NS_FAILED(rv)) return rv;| across two lines

+  } else {  // dir exists so lets try to use this dirs leaf as a name
the next bit of code has else before return, here's a rewrite
+    GetLeafName(getter_Copies(pathName));
+    newParent->Append(pathName);
+    newParent->Exists(&dirCheck);
+    if (dirCheck)
+      return NS_ERROR_FILE_ALREADY_EXISTS; // dest exists
+    newParent->Create(DIRECTORY_TYPE, oldPerms);
+  }

timeless.carp("Confusing while usage");
+  while (dirIterator->HasMoreElements(&hasMore), hasMore) {

+      entry->IsDirectory(&dirCheck);
+      entry->IsSymlink(&isSymlink);
+        if (dirCheck && !isSymlink) {
+          nsCOMPtr<nsIFile> destClone;
+          rv = newParent->Clone(getter_AddRefs(destClone));
+          if (NS_SUCCEEDED(rv)) {
+            nsCOMPtr<nsILocalFile> newDir(do_QueryInterface(destClone));
+            nsXPIDLCString leafName;
+            entry->GetLeafName(getter_Copies(leafName));
+            newDir->Append(leafName);
shouldn't this:
+            entry->CopyTo(newDir, nsnull);
be CopyToDirectory?
+          }

@@ -96,11 +98,11 @@
looks like tab fixing, but the indentation looks bad. thanks for working on 
this, I hope my comments are vaguely useful.

Comment 10

[Pete Collins (MDG)]

Attachment: updated patch

Comment 11

[Pete Collins (MDG)]

> you use |if(| and |if (| i think the code prefers the latter, i know i do.

Fixed.

> please split things like |if (NS_FAILED(rv)) return rv;| across two lines

Fixed

> the next bit of code has else before return,

Fixed

> timeless.carp("Confusing while usage");

Leaving, i see this type of iteration all over mozilla codebase. 

> shouldn't this:
+            entry->CopyTo(newDir, nsnull);
> be CopyToDirectory?

No, CopyToDirectory is not exposed to idl and can't be called from there. This
method is here so CopyTo can be fully implemented on unix and thus now fully
implemented across all platforms.

> but the indentation looks bad

Fixed

Can i get an r= then?

Thanks

--pete

Comment 12

[Conrad Carlen (not reading bugmail)]

(1)
-        // XXXbe what's the bugzilla.mozilla.org bug number for this?
-        rv = NS_ERROR_NOT_IMPLEMENTED;
+        if (newName)
+          newParent->Append(newName);
+        rv = CopyToDirectory(newParent);


If newName is not NULL, it's supposed to rename the copied dir, not affect where
it gets copied to, as this call to Append() will.

(2) In CopyToDirectory, there's no error checking on calls to IsDirectory,
Equals, and Create. Without careful error checking in something like this, if
something went wrong, you could end up dumping a huge number of files in the
wrong place.

(3) Should CopyToDirectory be named CopyDirectoryTo?

Comment 13

[Pete Collins (MDG)]

Attachment: added NS_FAILED tests IsDirectory, Equals and CREATE

Comment 14

[Pete Collins (MDG)]

> If newName is not NULL, it's supposed to rename the copied dir, not affect 
where it gets copied to, as this call to Append() will.


It does rename the copied dir.

For example if my current nsIFile is "/tmp" and i provied a newName param of
"foo" and my nsIFile destDir is "/home". 

CopyTo("/home", "foo");

It will copy the contents of tmp to /home/foo

This is exactly right. Or am i missing something here?

--pete

Comment 15

[Pete Collins (MDG)]

(3) Should CopyToDirectory be named CopyDirectoryTo?

I was just trying to keep with the current semantics. moveTo, copyTo.

In this case we are copying a dir. Hence copyToDirectory.

I'll call it anything you want to get an r=.  ;-)

--pete

Comment 16

[Pete Collins (MDG)]

BTW: you can try any permutation of paths and this method will just "do the
right thing"

Ex: 
copyTo("/home/", null);
copyTo("/home/", "foo");
copyTo("/home/dirDoesntExist", null);
copyTo("/home/dirDoesntExist", "doesntExistEither");
copyTo("/home/dirDoesntExist/doesntExist", "doesntExistEither");

etc . . .

--pete

Comment 17

[Conrad Carlen (not reading bugmail)]

> CopyTo("/home", "foo");

> It will copy the contents of tmp to /home/foo

That's right - I guess I missed something.

There's still a call to Create(...) that's missing an error check (critical) and
a number of other calls as well. 

I won't dig in my heels about the name of CopyToDirectory but any copy within a
file system is a copy to [a] directory, whereas CopyDirectoryTo is clear about
what it does on first glance.

 

Comment 18

[Pete Collins (MDG)]

Attachment: Renamed method to CopyDirectoryTo and added NS_FAILED to every call

Comment 19

[Pete Collins (MDG)]

Attachment: missed one test

Comment 20

[Pete Collins (MDG)]

I added a check for NS_FAILED for every method call. 
Is this the correct way of doing things?

If so, then i will make sure i always use it.

Could this introduce any performance overhead?

--pete

Comment 21

[Pete Collins (MDG)]

There is one thing to note here.

If a param of an non existant dir is provided and a null param of newName, it
will use the last name provided by the non existant dest and *not* the leafName
of the current nsIFile. Which to me is correct. If the caller is saying, copy to
this non existant dir, then that means we want that as our new name.

If you want to keep the name of the curent nsIFile, then the dest must be a dir
that already exists.

Ex:

copyTo("/home", null);

--pete

Comment 22

[Conrad Carlen (not reading bugmail)]

> I added a check for NS_FAILED for every method call. 
> Is this the correct way of doing things?

Yes. In this case, imagine what could happen if you call Create(), it fails, and
then you copy to that bogus dir. Also, without checking the result code after an
accessor, what you get back could be garbage.

It's not a significant performance hit. It probably takes a few processor cycles
to check the result code compared to the relatively glacial time in which files
are copied.

To the last point: I think that the leaf name of the copied dir should always be
maintained unless a newName param is provided. The effect of the newName param
shouldn't have anything to do with whether the dest exists or not. I would think
that if newParent didn't exist, it would be created and the copy would happen
the same as if it were there. 

Comment 23

[Pete Collins (MDG)]

Attachment: added some more error checking and fixed so original leaf will be used if newName is null

Comment 24

[Pete Collins (MDG)]

Ok, this implementation will always use the leafName if the newName param in
copyTo is null.

Great review Conrad. 

How about an r= now?

--pete

Comment 25

[Conrad Carlen (not reading bugmail)]

Alright, I think that's got it. r=ccarlen

Comment 26

[Brendan Eich [:brendan]]

pete: looks good, but (you're not the first, alas) here's a nit: the modeline
and most of the code have 4-space indentation style, but CopyDirectoryTo has
2-space.  Can you fix without making too many overlong lines, and split the few
that do surpass the 80th column by way too much?

Non-nit: why does the while loop suppress errors calling entry->CopyTo?  I think
it should fail, but if you want it to keep on trucking in the face of likely
out-of-disk-space, it should at least remember to return a failure code.

Another nit: move declarations (e.g., nsXPIDLCString pathname) down to right
before their uses, and within minimal block scopes.

/be

Comment 27

[Pete Collins (MDG)]

Marking 0.9.4 I won't get this in in time for 0.9.3

--pete

Comment 28

[Pete Collins (MDG)]

Attachment: updated patch as to Brendans comments

Comment 29

[Brendan Eich [:brendan]]

Almost, but

+                    entry->CopyTo(newDir, nsnull);
+                }
+            } else {
+                  entry->CopyTo(newParent, nsnull);
+

still ignores failure in an entry->CopyTo call, and the else clause's statement
is overindented.

/be

Comment 30

[Pete Collins (MDG)]

Attachment: fixed patch to return when we are out of memory

Comment 31

[Pete Collins (MDG)]

Brendan, I made a couple of chagnes on this patch. 

First i compacted the code some more and tightened up the style.

After doing lots of testing copying my /usr partition on to my (small) /root
partition, this method now does the right thing.

We still need the "continue" statements in there to accomodate situations like
dangling symlinks and locked files. So when we encounter them, we keep on
chugging issuing an NS_WARNING.

I also found some other unrelated bugs in this code such as IsDirectory not
returning true if dir is a symlink and copying of symlinks doesn't work properly.

I will file these seperately.

Let me know if this implementation is solid now.

Thanks

--pete

Comment 32

[Pete Collins (MDG)]

BTW: w/ this implementation, moveTo works quite nicely when moving entire dirs
across file systems. We previously couldn't do this on unix.

--pete

Comment 33

[Pete Collins (MDG)]

Brendan can you please sr this so i can check it in.

Thanks

--pete

Comment 34

[Brendan Eich [:brendan]]

pete: sorry for the delay -- I'll review this by tomorrow, quick notes and nits
first:

- The rv=... style is not "When in Rome" -- how about spaces around = ?

- Don't use NS_WARNING in DEBUG builds for a condition that has nothing to do
with internal program inconsistencies, or other debug-worthy events.  I think it
may be ok to keep on chugging, now that you point it out.  We can't fail half
way (no transactionality).  We can't usefully issue a (English, l10n problems)
warning in release as well as in debug.  So I think I agree with your continue
usage.

- Beware else after return non-sequiturs.

More soon, feel free to post a revised patch.

/be

Comment 35

[Pete Collins (MDG)]

Attachment: updated patch

Comment 36

[Pete Collins (MDG)]

Brendan i opted for a printf for debug builds. I want some kind of output when
there is a problem. 

cp -r will do the same thing, issue a warning.

--pete

Comment 37

[Brendan Eich [:brendan]]

This can't compile:

+    if NS_FAILED((rv = IsDirectory(&dirCheck)))
+        return rv;

I noticed pathName got moved into inner blocks, which is cool:

+    } else {  // dir exists lets try to use leaf
+        nsXPIDLCString pathName;
+        if (NS_FAILED(rv = GetLeafName(getter_Copies(pathName))))

but should you call this nsXPIDLCString leafName instead, as that's what it stores?

If you're going to use continue in this loop, you should use it for abnormal
cases such as the iterator's GetNext failing after a successful HasMoreElements,
here:

+        rv = dirIterator->GetNext((nsISupports**)getter_AddRefs(entry));
+        if (NS_SUCCEEDED(rv)) {

make that if be:

         if (NS_FAILED(rv))
             continue;

and then the rest of the loop body can be less indented.

Otherwise, looking pretty good -- one more patch?  Thanks for your patience.

/be

Comment 38

[Pete Collins (MDG)]

Attachment: full patch this time (last was missing code from header file)

Comment 39

[Pete Collins (MDG)]

Attachment: full patch w/ continue addition (less indentation)

Comment 40

[Pete Collins (MDG)]

Brendan, now that we have the full patch here, i'd say apply it and give it a
spin.  

I find copying /tmp to /home is a good test because you may encounter some
locked files along the way:

Operation not supported: /tmp/.X11-unix/X0
Operation not supported: /tmp/mysql.sock


--pete

Comment 41

[Brendan Eich [:brendan]]

I'll trust ya on the testing front.  Just move hasMore's declaration down to
just before its first use, in the while loop control, and sr=[e-mail]
(no need for another attachment).

/be

Comment 42

[Pete Collins (MDG)]

checked in marking FIXED

--pete