[prev in list] [next in list] [prev in thread] [next in thread] 

List:       coreutils-bug
Subject:    bug#14763: mv directory cross-filesystem where destination exists fails to remove dest with EISDIR
From:       Ken Booth <ken () booths ! org ! uk>
Date:       2013-07-24 21:50:55
Message-ID: 51F04C3F.6070904 () booths ! org ! uk
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]