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

Previous Next

Full log

View this message in rfc822 format

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