bug#14763: mv directory cross-filesystem where destination exists fails to remove dest with EISDIR

[Ken Booth]

Hi Eric,

Thanks for the reference to the POSIX standards.

The reason I wrote the patch the way I did was to emulate Solaris 
behaviour for a non-empty directory, however I read at
http://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html
the sentence "If /new/ names an existing directory, it shall be required 
to be an empty directory. "

Therefore I conclude that Solaris is not POSIX compliant.

After reading the instructions you suggested I hope the following patch 
is in the correct format, conforms to the requirements, and is also 
POSIX compliant ...

From 8b6549f321c06ee81262f58c6d7bd7e9c9092b30 Mon Sep 17 00:00:00 2001
From: Ken Booth <address@hidden>
Date: Tue, 2 Jul 2013 01:06:32 +0100
Subject: [PATCH] mv: if inter-device move target is directory use rmdir, not
 unlink

---
 src/copy.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index c1c8273..5137f27 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2173,13 +2173,27 @@ copy_internal (char const *src_name, char const 
*dst_name,
       /* The rename attempt has failed.  Remove any existing destination
          file so that a cross-device 'mv' acts as if it were really using
          the rename syscall.  */
-      if (unlink (dst_name) != 0 && errno != ENOENT)
+      if (S_ISDIR (src_mode))
         {
-          error (0, errno,
-             _("inter-device move failed: %s to %s; unable to remove 
target"),
-                 quote_n (0, src_name), quote_n (1, dst_name));
-          forget_created (src_sb.st_ino, src_sb.st_dev);
-          return false;
+          if (rmdir (dst_name) != 0 && errno != ENOENT)
+            {
+              error (0, errno,
+                 _("inter-device move failed: %s to %s; unable to 
remove target directory"),
+                     quote_n (0, src_name), quote_n (1, dst_name));
+              forget_created (src_sb.st_ino, src_sb.st_dev);
+              return false;
+            }
+        }
+      else
+        {
+          if (unlink (dst_name) != 0 && errno != ENOENT)
+            {
+              error (0, errno,
+                 _("inter-device move failed: %s to %s; unable to 
remove target"),
+                     quote_n (0, src_name), quote_n (1, dst_name));
+              forget_created (src_sb.st_ino, src_sb.st_dev);
+              return false;
+            }
         }

       new_dst = true;
--
1.8.1.4

I've tested it works with an empty directory, and produces the following 
error with a non-empty directory

/home/ken/git/fedora/coreutils/src/mv: inter-device move failed: 
‘/home/test’ to ‘/test’; unable to remove target directory: Directory 
not empty

I'm not sure of the etiquette here, but should I push the change, or 
leave that to a maintainer?

Regards,
Ken.

On 01/07/13 23:41, Eric Blake wrote:
> On 07/01/2013 04:34 PM, Eric Blake wrote:
> > > > If the destination path exists, mv shall attempt to remove it. If this fails 
> > > > for any reason, mv shall write a diagnostic message to standard error, do 
> > > > nothing more with the current source_file, and go on to any remaining 
> > > > source_files.
> > Aha - we are attempting to remove it with unlink().  But for empty
> > directories, we should be using rmdir(), or even better, we should be
> > using remove() (which subsumes both unlink() and rmdir() at once).  I
> > wonder if a simpler patch would be to just s/unlink/rmdir/ in the line
> > of code where you were adding special-casing on errno values.
> Then again, we DON'T want to replace a non-directory with a directory
> (or vice-versa, we don't want to replace an empty directory with a
> non-directory); so maybe it pays to be more careful about explicitly
> using unlink() vs. rmdir() on the destination (and not the shortcut of
> remove() which does not care about type), all based on what file type we
> already know that the source is, so that we can give the same sorts of
> failures as rename() would give on a local file system when attempting a
> cross-file-type rename.