Jim Meyering wrote: > The above wasn't quite right in that it failed to honor mv's --backup > option. mv --backup s f would not have created the required backup file. > I've adjusted it to fix that, and added tests to cover both cases. > This is still not quite ready (i.e., the FIXME comment, where I'll add > some explanation), but otherwise should be fine. > > Date: Wed, 1 Feb 2012 21:42:45 +0100 > Subject: [PATCH] mv: "mv A B" would sometimes succeed, yet A would remain, > ...
I address the FIXME and tweaked the comment: (A,B) seemed a little clearer than (s1,s2). I've also tightened up the test, regarding --backup and existence of backup files. >From de817cfa6e780323b3eac1f92ac81e8c7871d088 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyer...@redhat.com> Date: Wed, 1 Feb 2012 21:42:45 +0100 Subject: [PATCH] mv: "mv A B" would sometimes succeed, yet A would remain, ... But only when both A and B were hard links to the same symlink. * src/copy.c (same_file_ok): Handle another special case: the one in which we are moving a symlink onto a hard link to itself. In this case, we must explicitly tell the caller to unlink the source file. Otherwise, at least the linux-3.x kernel rename function would do nothing, as mandated by POSIX 2008. * tests/mv/symlink-onto-hardlink-to-self: New test. * tests/Makefile.am (TESTS): Add it. * NEWS (Bug fixes): Mention it. Reported by Bernhard Voelker in http://bugs.gnu.org/10686 --- NEWS | 5 +++ src/copy.c | 29 +++++++++++++++-- tests/Makefile.am | 1 + tests/mv/symlink-onto-hardlink-to-self | 56 ++++++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 3 deletions(-) create mode 100755 tests/mv/symlink-onto-hardlink-to-self diff --git a/NEWS b/NEWS index 9eebbf6..ca8c0b5 100644 --- a/NEWS +++ b/NEWS @@ -11,6 +11,11 @@ GNU coreutils NEWS -*- outline -*- referent, there is no risk of data loss, since the symlink will typically still point to one of the hard links. + "mv A B" could succeed, yet A would remain. This would happen only when + both A and B were hard links to the same symlink, and with a kernel for + which rename("A","B") would do nothing and return 0. Now, in this + unusual case, mv does not call rename, and instead simply unlinks A. + * Noteworthy changes in release 8.15 (2012-01-06) [stable] diff --git a/src/copy.c b/src/copy.c index 4810de8..541ca20 100644 --- a/src/copy.c +++ b/src/copy.c @@ -1226,10 +1226,33 @@ same_file_ok (char const *src_name, struct stat const *src_sb, same_link = same; /* If both the source and destination files are symlinks (and we'll - know this here IFF preserving symlinks), then it's ok -- as long - as they are distinct. */ + know this here IFF preserving symlinks), then it's usually ok + when they are distinct. */ if (S_ISLNK (src_sb->st_mode) && S_ISLNK (dst_sb->st_mode)) - return ! same_name (src_name, dst_name); + { + bool sn = same_name (src_name, dst_name); + if ( ! sn) + { + /* It's fine when we're making any type of backup. */ + if (x->backup_type != no_backups) + return true; + + /* Here we have two symlinks that are hard-linked together, + and we're not making backups. In this unusual case, simply + returning true would lead to mv calling "rename(A,B)", + which would do nothing and return 0. I.e., A would + not be removed. Hence, the solution is to tell the + caller that all it must do is unlink A and return. */ + if (same_link) + { + *unlink_src = true; + *return_now = true; + return true; + } + } + + return ! sn; + } src_sb_link = src_sb; dst_sb_link = dst_sb; diff --git a/tests/Makefile.am b/tests/Makefile.am index a94aaa2..c951f69 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -492,6 +492,7 @@ TESTS = \ mv/partition-perm \ mv/perm-1 \ mv/symlink-onto-hardlink \ + mv/symlink-onto-hardlink-to-self \ mv/to-symlink \ mv/trailing-slash \ mv/update \ diff --git a/tests/mv/symlink-onto-hardlink-to-self b/tests/mv/symlink-onto-hardlink-to-self new file mode 100755 index 0000000..efaff66 --- /dev/null +++ b/tests/mv/symlink-onto-hardlink-to-self @@ -0,0 +1,56 @@ +#!/bin/sh +# Demonstrate that when moving a symlink onto a hardlink-to-that-symlink, the +# source symlink is removed. Depending on your kernel (e.g., with the linux +# kernel), prior to coreutils-8.16, the mv would successfully perform a no-op. +# I.e., surprisingly, mv s1 s2 would succeed, yet fail to remove s1. + +# Copyright (C) 2012 Free Software Foundation, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +. "${srcdir=.}/init.sh"; path_prepend_ ../src +print_ver_ mv + +# Create a file f, and a symlink s1 to that file. +touch f || framework_failure_ +ln -s f s2 || framework_failure_ + +for opt in '' --backup; do + + # Attempt to create a hard link to that symlink. + # On some systems, it's not possible: they create a hard link to the referent. + ln s2 s1 || framework_failure_ + + # If s1 is not a symlink, skip this test. + test -h s1 \ + || skip_ your kernel or file system cannot create a hard link to a symlink + + mv $opt s1 s2 > out 2>&1 || fail=1 + compare /dev/null out || fail=1 + + # Ensure that s1 is gone. + test -e s1 && fail=1 + + if test "$opt" = --backup; then + # With --backup, ensure that the backup file was created. + ref=$(readlink s2~) || fail=1 + test "$ref" = f || fail=1 + else + # Without --backup, ensure there is no backup file. + test -e s2~ && fail=1 + fi + +done + +Exit $fail -- 1.7.9.190.gf1d33
