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

[Ken Booth]

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