Re: [PATCH] cp: do not create empty dst file if failed to make reflink

[Pádraig Brady]

On 03/11/2013 07:05 PM, Guangyu Sun wrote:
> If making reflink across devices or if the filesystem does not support
> reflink, we want it to return an error and do nothing else. However,
> now it will create a new empty file to the dst. Fix it.
> 
> test case in an ext4 filesystem:
> $ ls
> foo
> $ cp --reflink foo bar
> cp: failed to clone `bar': Inappropriate ioctl for device
> $ ls
> foo     bar
> $
> 
> Signed-off-by: Guangyu Sun <address@hidden>
> ---
>  src/copy.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/copy.c b/src/copy.c
> index 5c0ee1e..b323876 100644
> --- a/src/copy.c
> +++ b/src/copy.c
> @@ -1176,6 +1176,9 @@ close_src_and_dst_desc:
>        error (0, errno, _("failed to close %s"), quote (dst_name));
>        return_val = false;
>      }
> +  if (! return_val && *new_dst)
> +    if (unlink (dst_name))
> +      error (0, errno, _("cannot remove %s"), quote (dst_name));
>  close_src_desc:
>    if (close (source_desc) < 0)
>      {

This is an awkward case.
Your patch as is will handle your specific case,
but also unlink() the dest in other cases where
a partial copy is done, which you might not want.

Whether to leave partial copies in place is a tricky issue:
http://lists.gnu.org/archive/html/bug-coreutils/2008-06/threads.html#00146
http://bugs.gnu.org/10055

Taking your approach, where we unlink on (error and new dest file),
might be OK, but it isn't general either, as it doesn't handle
the case where cp itself is interrupted due to Ctrl-C, reboot, ...
Nor does it handle the case where we're truncating existing files.
The only general solution is to repeat the operation until no errors,
or otherwise inspect and cleanup the destination.
So if we need logic to do that outside of cp, then is it worth
adding such logic within cp?

Also considering the specific case of reflink(), perhaps the failure
is partial and in that case we might not want to unlink() the dest.
I.E. if we did add such logic to cp, we might want to restrict it
to (errno==EXDEV || errno==NOTTY || errno==ENOTSUP).

Taking the case of a single file, the issue can be handled outside of cp:
  cp --reflink foo /bar || rm -f /bar/foo
Or partially within cp if you want to revert to a normal copy:
  cp --reflink=auto foo /bar || rm -f /bar/foo

If copying many files, then there is also the issue that even
if we do unlink the files, we'll leave empty dirs in the dest
for the copied hierarchy.
This is another case which "|| rm ..." above handles.

thanks,
Pádraig.