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 <[email protected]> > --- > 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.
