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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 14763 in the body.
You can then email your comments to 14763 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ken Booth <ken <at> booths.org.uk>
To: bug-coreutils <at> gnu.org
Cc: Ken Booth <kbooth <at> redhat.com>
Subject: mv directory cross-filesystem where destination exists fails to remove
 dest with EISDIR
Date: Mon, 01 Jul 2013 22:21:36 +0100

Hi,

I have found a bug which affects RHEL 5.9, RHEL 6.4, Fedora 18, and the 
latest version of coreutils from the git repo this morning (1st July 
2013) in exactly the same way, and yet does not affect Solaris 10's mv 
command.

Test case:

f18 # mkdir /test /home/test
f18 # cp /etc/passwd /home/test
f18 # mv /home/test /
mv: overwrite ‘/test’? y
mv: inter-device move failed: ‘/home/test’ to ‘/test’; unable to remove 
target: Is a directory

Actual results:
mv: inter-device move failed: ‘/home/test’ to ‘/test’; unable to remove 
target: Is a directory

Expected results:
/home/test/passwd is copied to /test/passwd

I have determined that the problem can be "fixed" (made to behave the 
same as Solaris) by editting src/copy.c as follows:

2176c2176
< if (unlink (dst_name) != 0 && errno != ENOENT)
---
>       if (unlink (dst_name) != 0 &&  errno != ENOENT && errno != EISDIR)
2267,2269c2267,2278
< error (0, errno, _("cannot create directory %s"),
< quote (dst_name));
< goto un_backup;
---
> if (errno == EEXIST)
> {
> if (lchmod (dst_name, dst_mode & ~omitted_permissions) != 0)
> {
> error (0, errno, _("setting permissions for %s"),
> quote (dst_name));
> }
> } else {
> error (0, errno, _("cannot create directory %s"),
> quote (dst_name));
> goto un_backup;
> }

This has the same effect as mv does on Solaris 10.

Where the destination directory exists on the new mount point, the 
permissions are modified and new files will overwrite existing files in 
the destination tree. However, unique files in the destination directory 
will not be removed.

The BIG question is:
There is obviously a bug in the GNU mv command as its failing to unlink 
the directory, however ...

should the GNU mv command behave the same way as the Solaris mv commnd?

e.g.
f18# ls -la /home/test /test
/home/test:
total 12
drwxr-sr-x. 2 root root 4096 Jul 1 21:18 .
drwxr-xr-x. 14 root root 4096 Jul 1 21:18 ..
-rw-r--r--. 1 root root 2768 Jul 1 21:18 passwd

/test:
total 16
drwxr-xr-x. 2 root root 4096 Jul 1 21:20 .
dr-xr-xr-x. 25 root root 4096 Jul 1 21:09 ..
-rw-r--r--. 1 root root 1162 Jul 1 12:53 group
-rw-r--r--. 1 root root 2777 Jul 1 21:20 passwd
f18# /home/ken/git/fedora/coreutils/src/mv /home/test /
f18# ls -la /home/test /test
ls: cannot access /home/test: No such file or directory
/test:
total 16
drwxr-sr-x. 2 root root 4096 Jul 1 21:18 .
dr-xr-xr-x. 25 root root 4096 Jul 1 21:09 ..
-rw-r--r--. 1 root root 1162 Jul 1 12:53 group
-rw-r--r--. 1 root root 2768 Jul 1 21:18 passwd

or should we really be deleting the directory as defined by the man page 
wording "overwrite" and observed by the failed attempt at unlinking ?

e.g.
should we delete /test/group

Which behaviour is correct?

Regards,
Ken Booth
Red Hat UK Ltd

PS: There is a Red Hat bugzilla for this
https://bugzilla.redhat.com/show_bug.cgi?id=980061

Message #8 received at 14763 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: ken <at> booths.org.uk
Cc: 14763 <at> debbugs.gnu.org, Ken Booth <kbooth <at> redhat.com>
Subject: Re: bug#14763: mv directory cross-filesystem where destination exists
 fails to remove dest with EISDIR
Date: Mon, 01 Jul 2013 16:27:23 -0600

[Message part 1 (text/plain, inline)]

On 07/01/2013 03:21 PM, Ken Booth wrote:
> Hi,
> 
> I have found a bug which affects RHEL 5.9, RHEL 6.4, Fedora 18, and the
> latest version of coreutils from the git repo this morning (1st July
> 2013) in exactly the same way, and yet does not affect Solaris 10's mv
> command.
> 
> Test case:
> 
> f18 # mkdir /test /home/test
> f18 # cp /etc/passwd /home/test
> f18 # mv /home/test /
> mv: overwrite ‘/test’? y
> mv: inter-device move failed: ‘/home/test’ to ‘/test’; unable to remove
> target: Is a directory

Let's read what POSIX has to say about this:
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/mv.html

We are using the second form (mv source_file... target_dir) with
source_file of /home/test and target_dir of /.  The destination path is
thus constructed to be "/test".

Step 1 says:

>>     If the destination path exists, the -f option is not specified, and either of the following conditions is true:
>> 
>>         The permissions of the destination path do not permit writing and the standard input is a terminal.
>> 
>>         The -i option is specified.
>> 
>>     the mv utility shall write a prompt to standard error and read a line from standard input. If the response is not affirmative, mv shall do nothing more with the current source_file and go on to any remaining source_files.

The destination "/test" exists, but permissions allow writing (well,
assuming your umask is sane when you did mkdir) and -i is not specified,
so this step does not stop us, and we go on to step 2.

>> If the source_file operand and destination path name the same existing file...

Not the same, so on to step 3.

>> The mv utility shall perform actions equivalent to the rename() function defined in the System Interfaces volume of POSIX.1-2008, called with the following arguments:
>> 
>>     The source_file operand is used as the old argument.
>> 
>>     The destination path is used as the new argument.
>> 
>> If this succeeds, mv shall do nothing more with the current source_file and go on to any remaining source_files. If this fails for any reasons other than those described for the errno [EXDEV] in the System Interfaces volume of POSIX.1-2008, 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.

so now we have to cross-reference to see what is required for
rename("/home/test", "/test"):
http://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html

>> If the old argument points to the pathname of a directory, the new argument shall not point to the pathname of a file that is not a directory. If the directory named by the new argument exists, it shall be removed and old renamed to new. In this case, a link named new shall exist throughout the renaming operation and shall refer either to the directory referred to by new or old before the operation began. If new names an existing directory, it shall be required to be an empty directory.

Well, we just proved by your above construction that /test is empty, and
therefore should silently be replaced by the (non-empty) directory that
used to be named /home/test.  Therefore, the rename() should succeed,
and mv should do nothing further for the /home/test argument; and the
result of the rename is that the former /home/test/passwd should now be
located at /test/passwd.  You are correct that GNU mv has a bug.

Hmm, I seem to recall reporting a similar bug in the past regarding the
behavior on symlinks - a bit of research turned up commit f1d1ee912 in
May 2006 regarding non-atomic rename() of symlinks; now we are dealing
with non-atomic rename() of empty destination directories.

> Actual results:
> mv: inter-device move failed: ‘/home/test’ to ‘/test’; unable to remove
> target: Is a directory
> 
> Expected results:
> /home/test/passwd is copied to /test/passwd

Concur that this is what POSIX requires.

> 
> I have determined that the problem can be "fixed" (made to behave the
> same as Solaris) by editting src/copy.c as follows:
> 
> 2176c2176
> < if (unlink (dst_name) != 0 && errno != ENOENT)
> ---
>>       if (unlink (dst_name) != 0 &&  errno != ENOENT && errno != EISDIR)

Ed script patches are illegible.  Please instead submit this as a
context diff.  These directions in HACKING may help:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=blob;f=HACKING;h=49f97381;hb=HEAD

As is, I don't think your ed script patch is correct - it is not atomic.
 In other words, I don't think we should be trying unlink() followed by
special-case handling based on certain errno values, but instead should
be trying rename() up front.  Since the results are required to be
atomic, the success or failure of rename() will tell us whether we the
source directory was correctly renamed atop a previously empty
destination directory.

> 
> This has the same effect as mv does on Solaris 10.
> 
> Where the destination directory exists on the new mount point, the
> permissions are modified and new files will overwrite existing files in
> the destination tree. However, unique files in the destination directory
> will not be removed.

Huh? Nothing in the POSIX wording talked about altering permissions of
the destination directory after a rename of source dir to empty target dir.

> PS: There is a Red Hat bugzilla for this
> https://bugzilla.redhat.com/show_bug.cgi?id=980061

Thanks for the pointer.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #11 received at 14763 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
Cc: 14763 <at> debbugs.gnu.org, ken <at> booths.org.uk, Ken Booth <kbooth <at> redhat.com>
Subject: Re: bug#14763: mv directory cross-filesystem where destination exists
 fails to remove dest with EISDIR
Date: Mon, 01 Jul 2013 16:34:47 -0600

[Message part 1 (text/plain, inline)]

On 07/01/2013 04:27 PM, Eric Blake wrote:

> Not the same, so on to step 3.
> 
>>> The mv utility shall perform actions equivalent to the rename() function defined in the System Interfaces volume of POSIX.1-2008, called with the following arguments:
>>>
>>>     The source_file operand is used as the old argument.
>>>
>>>     The destination path is used as the new argument.
>>>
>>> If this succeeds, mv shall do nothing more with the current source_file and go on to any remaining source_files. If this fails for any reasons other than those described for the errno [EXDEV] in the System Interfaces volume of POSIX.1-2008, 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.
> 
> so now we have to cross-reference to see what is required for
> rename("/home/test", "/test"):
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html
> 
>>> If the old argument points to the pathname of a directory, the new argument shall not point to the pathname of a file that is not a directory. If the directory named by the new argument exists, it shall be removed and old renamed to new. In this case, a link named new shall exist throughout the renaming operation and shall refer either to the directory referred to by new or old before the operation began. If new names an existing directory, it shall be required to be an empty directory.
> 
> Well, we just proved by your above construction that /test is empty, and
> therefore should silently be replaced by the (non-empty) directory that
> used to be named /home/test.  Therefore, the rename() should succeed,

Oops, I missed one point - rename() is allowed to fail cross-filesystem
with EXDEV, and that's part of your testcase (at least, I assume you
mean that /test and /home/test are different filesystems, and therefore
the atomic rename() fails and we have to plow on with the next steps in
POSIX).  So, because the failure is EXDEV, we go on to step 4:

>> If the destination path exists, and it is a file of type directory and source_file is not a file of type directory, or it is a file not of type directory and source_file is a file of type directory, 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. If the destination path exists and was created by a previous step, it is unspecified whether this will treated as an error or the destination path will be overwritten.

The destination exists, but we don't have mismatch between
directory/non-directory.  So we don't error out, but instead go on to
step 5:

>> 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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #14 received at 14763 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
Cc: 14763 <at> debbugs.gnu.org, ken <at> booths.org.uk, Ken Booth <kbooth <at> redhat.com>
Subject: Re: bug#14763: mv directory cross-filesystem where destination exists
 fails to remove dest with EISDIR
Date: Mon, 01 Jul 2013 16:41:07 -0600

[Message part 1 (text/plain, inline)]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #17 received at 14763 <at> debbugs.gnu.org (full text, mbox):

From: Ken Booth <ken <at> booths.org.uk>
To: Eric Blake <eblake <at> redhat.com>
Cc: 14763 <at> debbugs.gnu.org, Ken Booth <kbooth <at> redhat.com>
Subject: Re: bug#14763: mv directory cross-filesystem where destination exists
 fails to remove dest with EISDIR
Date: Tue, 02 Jul 2013 01:29:44 +0100

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 <ken <at> booths.org.uk>
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.
>

Message #22 received at 14763-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: ken <at> booths.org.uk
Cc: 14763-done <at> debbugs.gnu.org, Eric Blake <eblake <at> redhat.com>,
 Ken Booth <kbooth <at> redhat.com>
Subject: Re: bug#14763: mv directory cross-filesystem where destination exists
 fails to remove dest with EISDIR
Date: Wed, 24 Jul 2013 18:14:29 +0100

On 07/02/2013 01:29 AM, Ken Booth wrote:
> 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 <ken <at> booths.org.uk>
> 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;

This looks good, thanks.
Though I don't think there's a need to duplicate the blocks above:
One could minimize to:

-      if (unlink (dst_name) != 0 && errno != ENOENT)
+      if ((S_ISDIR (src_mode) ? rmdir : unlink) (dst_name) != 0
+          && errno != ENOENT)

Though I think unlink at least can be a function like macro,
and so the above could cause compile issues on some platforms.
Therefore I'm adjusting to the following equivalent and will then push:

-      if (unlink (dst_name) != 0 && errno != ENOENT)
+      if ((S_ISDIR (src_mode) ? rmdir (dst_name) : unlink (dst_name)) != 0
+          && errno != ENOENT)

Note we're checking the type of src which is a bit confusing
since we're removing dst.  Now we could check dst as the
overwrite dir with non dir case is checked for earlier (and vice versa).
But I guess this is a double check for this case.
I'll add a comment along these lines.

I'll add a test too.

thanks,
Pádraig.

Message #25 received at 14763-done <at> debbugs.gnu.org (full text, mbox):

From: Ken Booth <ken <at> booths.org.uk>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 14763-done <at> debbugs.gnu.org, Eric Blake <eblake <at> redhat.com>,
 Ken Booth <kbooth <at> redhat.com>
Subject: Re: bug#14763: mv directory cross-filesystem where destination exists
 fails to remove dest with EISDIR
Date: Wed, 24 Jul 2013 22:50:55 +0100

On 24/07/13 18:14, Pádraig Brady wrote:
> On 07/02/2013 01:29 AM, Ken Booth wrote:
>> 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 <ken <at> booths.org.uk>
>> 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;
> This looks good, thanks.
> Though I don't think there's a need to duplicate the blocks above:
> One could minimize to:
>
> -      if (unlink (dst_name) != 0 && errno != ENOENT)
> +      if ((S_ISDIR (src_mode) ? rmdir : unlink) (dst_name) != 0
> +          && errno != ENOENT)
>
> Though I think unlink at least can be a function like macro,
> and so the above could cause compile issues on some platforms.
> Therefore I'm adjusting to the following equivalent and will then push:
>
> -      if (unlink (dst_name) != 0 && errno != ENOENT)
> +      if ((S_ISDIR (src_mode) ? rmdir (dst_name) : unlink (dst_name)) != 0
> +          && errno != ENOENT)
>
> Note we're checking the type of src which is a bit confusing
> since we're removing dst.  Now we could check dst as the
> overwrite dir with non dir case is checked for earlier (and vice versa).
> But I guess this is a double check for this case.
> I'll add a comment along these lines.
>
> I'll add a test too.
>
> thanks,
> Pádraig.
>
Hi Pádraig,

I thought about the fact that we're only checking src and decided its 
the correct behaviour, as we intend to get an error if dst is the wrong 
type. I should have included a comment to that effect, just to make it 
clear that we dont want to check dst.

So the code as you've written it should stand without an extra check, 
just a comment.

Thanks for approving my first patch.

Regards,
Ken.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.