Re: bug#69532: mv's new -x option should be made orthogonal to -t/-T/default

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2024-03-05 06:16, Pádraig Brady wrote:
I think I'll remove the as yet unreleased mv --swap from coreutils, given that
util-linux is as widely available as coreutils on GNU/Linux platforms.

Although removing that "mv --swap" implementation was a win, I don't think we can simply delegate this to util-linux's exch command. Exchanging files via a renameat-like call is not limited to the Linux kernel; it's also possible on macOS via renameatx_np with RENAME_SWAP, and there have been noises about adding similar things to other operating systems.

I just now added support for macOS renameatx_np to Gnulib, so coreutils does not need to worry about the macOS details; it can simply use renameatu with the Linux flags. See:

https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=af32ee824ee18255839f9812b8ed61aa5257a82b

Even with Linux it's dicey. People may have older util-linux installed and so lack the 'exch' utility; this is true for both Fedora 39 and Ubuntu 23.10, the current releases. Ubuntu is also odd in that it doesn't install all the util-linux utilities as part of the util-linux package, so it's not clear what they will do with 'exch'.

So I propose that we implement the idea in coreutils in a better way, that interacts more nicely with -t, -T, etc. Also, I suggest using the Linuxish name "--exchange" instead of the macOSish name "--swap", and (for now at least) not giving the option a single-letter equivalent as I expect it to be useful from scripts, not interactively.

After looking at various ways to do it I came up with the attached proposed patch. This should work on both GNU/Linux and macOS, if your OS is recent enough and the file system supports atomic exchange.
From d522aba06107d3532ad6103470727bf9057f8d2c Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@xxxxxxxxxxx>
Date: Sat, 16 Mar 2024 22:50:17 -0700
Subject: [PATCH] mv: new option --exchange

* src/copy.h (struct cp_options): New member 'exchange'.
* src/copy.c (copy_internal): Support the new member.
* src/mv.c (EXCHANGE_OPTION): New constant.
(long_options): Add --exchange.
(usage): Document --exchange.
(main): Support --exchange.
* tests/mv/mv-exchange.sh: New test case.
* tests/local.mk (all_tests): Add it.
---
 NEWS                    |  7 ++++++
 doc/coreutils.texi      | 18 ++++++++++++++
 src/copy.c              | 54 +++++++++++++++++++++++------------------
 src/copy.h              |  4 +++
 src/mv.c                | 16 +++++++++---
 tests/local.mk          |  1 +
 tests/mv/mv-exchange.sh | 41 +++++++++++++++++++++++++++++++
 7 files changed, 114 insertions(+), 27 deletions(-)
 create mode 100755 tests/mv/mv-exchange.sh

diff --git a/NEWS b/NEWS
index f21efc7c0..67bb27ebb 100644
--- a/NEWS
+++ b/NEWS
@@ -81,6 +81,13 @@ GNU coreutils NEWS                                    -*- outline -*-
   and the command exits with failure status if existing files.
   The -n,--no-clobber option is best avoided due to platform differences.
 
+  mv now accepts an --exchange option, which causes the source and
+  destination to be exchanged.  It should be combined with
+  --no-target-directory (-T) if the destination is a directory.
+  The exchange is atomic if source and destination are on a single
+  file system that supports atomic exchange; --exchange is not yet
+  supported in other situations.
+
   od now supports printing IEEE half precision floating point with -t fH,
   or brain 16 bit floating point with -t fB, where supported by the compiler.
 
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index d07ed7e76..c456a03d9 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -10269,6 +10269,24 @@ skip existing files but not fail.
 If a file cannot be renamed because the destination file system differs,
 fail with a diagnostic instead of copying and then removing the file.
 
+@item --exchange
+@opindex --exchange
+Exchange source and destination instead of renaming source to destination.
+Both files must exist; they need not be the same type.
+The exchange is atomic if the source and destination are both in a
+single file system that supports atomic exchange;
+exchanges are not yet supported in other situations.
+
+This option can be used to replace one directory with another, atomically.
+When used this way, it should be combined with
+@code{--no-target-directory} (@option{-T})
+to avoid confusion about the destination location.
+Also, if the two directories might not be on the same file system,
+using @code{--no-copy} will prevent future
+versions of @command{mv} from implementing the exchange by copying.
+For example, you might use @samp{mv -T --exchange --no-copy
+@var{d1} @var{d2}} to exchange the directories @var{d1} and @var{d2}.
+
 @item -u
 @itemx --update
 @opindex -u
diff --git a/src/copy.c b/src/copy.c
index 8d99f8562..e7bf6022f 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2223,9 +2223,11 @@ copy_internal (char const *src_name, char const *dst_name,
     {
       if (rename_errno < 0)
         rename_errno = (renameatu (AT_FDCWD, src_name, dst_dirfd, drelname,
-                                   RENAME_NOREPLACE)
+                                   (x->exchange
+                                    ? RENAME_EXCHANGE : RENAME_NOREPLACE))
                         ? errno : 0);
-      nonexistent_dst = *rename_succeeded = rename_errno == 0;
+      *rename_succeeded = rename_errno == 0;
+      nonexistent_dst = *rename_succeeded && !x->exchange;
     }
 
   if (rename_errno == 0
@@ -2246,7 +2248,7 @@ copy_internal (char const *src_name, char const *dst_name,
 
       src_mode = src_sb.st_mode;
 
-      if (S_ISDIR (src_mode) && !x->recursive)
+      if (S_ISDIR (src_mode) && !x->recursive && !x->exchange)
         {
           error (0, 0, ! x->install_mode /* cp */
                  ? _("-r not specified; omitting directory %s")
@@ -2289,7 +2291,7 @@ copy_internal (char const *src_name, char const *dst_name,
      treated the same as nonexistent files.  */
   bool new_dst = 0 < nonexistent_dst;
 
-  if (! new_dst)
+  if (! new_dst && ! x->exchange)
     {
       /* Normally, fill in DST_SB or set NEW_DST so that later code
          can use DST_SB if NEW_DST is false.  However, don't bother
@@ -2657,7 +2659,7 @@ skip:
      Also, with --recursive, record dev/ino of each command-line directory.
      We'll use that info to detect this problem: cp -R dir dir.  */
 
-  if (rename_errno == 0)
+  if (rename_errno == 0 || x->exchange)
     earlier_file = nullptr;
   else if (x->recursive && S_ISDIR (src_mode))
     {
@@ -2752,7 +2754,7 @@ skip:
 
   if (x->move_mode)
     {
-      if (rename_errno == EEXIST)
+      if (rename_errno == EEXIST && !x->exchange)
         rename_errno = (renameat (AT_FDCWD, src_name, dst_dirfd, drelname) == 0
                         ? 0 : errno);
 
@@ -2781,7 +2783,7 @@ skip:
                  _destination_ dev/ino, since the rename above can't have
                  changed those, and 'mv' always uses lstat.
                  We could limit it further by operating
-                 only on non-directories.  */
+                 only on non-directories when !x->exchange.  */
               record_file (x->dest_info, dst_relname, &src_sb);
             }
 
@@ -2828,7 +2830,7 @@ skip:
          where you'd replace '18' with the integer in parentheses that
          was output from the perl one-liner above.
          If necessary, of course, change '/tmp' to some other directory.  */
-      if (rename_errno != EXDEV || x->no_copy)
+      if (rename_errno != EXDEV || x->no_copy || x->exchange)
         {
           /* There are many ways this can happen due to a race condition.
              When something happens between the initial follow_fstatat and the
@@ -2841,25 +2843,29 @@ skip:
              destination file are made too restrictive, the rename will
              fail.  Etc.  */
           char const *quoted_dst_name = quoteaf_n (1, dst_name);
-          switch (rename_errno)
-            {
-            case EDQUOT: case EEXIST: case EISDIR: case EMLINK:
-            case ENOSPC: case ETXTBSY:
+          if (x->exchange)
+            error (0, rename_errno, _("cannot exchange %s and %s"),
+                   quoteaf_n (0, src_name), quoted_dst_name);
+          else
+            switch (rename_errno)
+              {
+              case EDQUOT: case EEXIST: case EISDIR: case EMLINK:
+              case ENOSPC: case ETXTBSY:
 #if ENOTEMPTY != EEXIST
-            case ENOTEMPTY:
+              case ENOTEMPTY:
 #endif
-              /* The destination must be the problem.  Don't mention
-                 the source as that is more likely to confuse the user
-                 than be helpful.  */
-              error (0, rename_errno, _("cannot overwrite %s"),
-                     quoted_dst_name);
-              break;
+                /* The destination must be the problem.  Don't mention
+                   the source as that is more likely to confuse the user
+                   than be helpful.  */
+                error (0, rename_errno, _("cannot overwrite %s"),
+                       quoted_dst_name);
+                break;
 
-            default:
-              error (0, rename_errno, _("cannot move %s to %s"),
-                     quoteaf_n (0, src_name), quoted_dst_name);
-              break;
-            }
+              default:
+                error (0, rename_errno, _("cannot move %s to %s"),
+                       quoteaf_n (0, src_name), quoted_dst_name);
+                break;
+              }
           forget_created (src_sb.st_ino, src_sb.st_dev);
           return false;
         }
diff --git a/src/copy.h b/src/copy.h
index dfa9435b3..ab89c75fd 100644
--- a/src/copy.h
+++ b/src/copy.h
@@ -155,6 +155,10 @@ struct cp_options
      If that fails and NO_COPY, fail instead of copying.  */
   bool move_mode, no_copy;
 
+  /* Exchange instead of renaming.  Valid only if MOVE_MODE and if
+     BACKUP_TYPE == no_backups.  */
+  bool exchange;
+
   /* If true, install(1) is the caller.  */
   bool install_mode;
 
diff --git a/src/mv.c b/src/mv.c
index 9dc40fe3e..692943a70 100644
--- a/src/mv.c
+++ b/src/mv.c
@@ -48,6 +48,7 @@
 enum
 {
   DEBUG_OPTION = CHAR_MAX + 1,
+  EXCHANGE_OPTION,
   NO_COPY_OPTION,
   STRIP_TRAILING_SLASHES_OPTION
 };
@@ -67,6 +68,7 @@ static struct option const long_options[] =
   {"backup", optional_argument, nullptr, 'b'},
   {"context", no_argument, nullptr, 'Z'},
   {"debug", no_argument, nullptr, DEBUG_OPTION},
+  {"exchange", no_argument, nullptr, EXCHANGE_OPTION},
   {"force", no_argument, nullptr, 'f'},
   {"interactive", no_argument, nullptr, 'i'},
   {"no-clobber", no_argument, nullptr, 'n'},   /* Deprecated.  */
@@ -271,6 +273,9 @@ Rename SOURCE to DEST, or move SOURCE(s) to DIRECTORY.\n\
 "), stdout);
       fputs (_("\
       --debug                  explain how a file is copied.  Implies -v\n\
+"), stdout);
+      fputs (_("\
+      --exchange               exchange source and destination\n\
 "), stdout);
       fputs (_("\
   -f, --force                  do not prompt before overwriting\n\
@@ -361,6 +366,9 @@ main (int argc, char **argv)
         case DEBUG_OPTION:
           x.debug = x.verbose = true;
           break;
+        case EXCHANGE_OPTION:
+          x.exchange = true;
+          break;
         case NO_COPY_OPTION:
           x.no_copy = true;
           break;
@@ -469,7 +477,7 @@ main (int argc, char **argv)
   else
     {
       char const *lastfile = file[n_files - 1];
-      if (n_files == 2)
+      if (n_files == 2 && !x.exchange)
         x.rename_errno = (renameatu (AT_FDCWD, file[0], AT_FDCWD, lastfile,
                                      RENAME_NOREPLACE)
                           ? errno : 0);
@@ -514,11 +522,13 @@ main (int argc, char **argv)
       strip_trailing_slashes (file[i]);
 
   if (make_backups
-      && (x.interactive == I_ALWAYS_SKIP
+      && (x.exchange
+          || x.interactive == I_ALWAYS_SKIP
           || x.interactive == I_ALWAYS_NO))
     {
       error (0, 0,
-             _("--backup is mutually exclusive with -n or --update=none-fail"));
+             _("cannot combine --backup with "
+               "--exchange, -n, or --update=none-fail"));
       usage (EXIT_FAILURE);
     }
 
diff --git a/tests/local.mk b/tests/local.mk
index 7cd1ef7b5..f0ac0386f 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -698,6 +698,7 @@ all_tests =					\
   tests/mv/into-self-3.sh			\
   tests/mv/into-self-4.sh			\
   tests/mv/leak-fd.sh				\
+  tests/mv/mv-exchange.sh			\
   tests/mv/mv-n.sh				\
   tests/mv/mv-special-1.sh			\
   tests/mv/no-copy.sh				\
diff --git a/tests/mv/mv-exchange.sh b/tests/mv/mv-exchange.sh
new file mode 100755
index 000000000..485403a1d
--- /dev/null
+++ b/tests/mv/mv-exchange.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+# Test mv --exchange.
+
+# Copyright (C) 2024 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 <https://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ mv
+
+
+# Test exchanging files.
+touch a || framework_failure_
+mkdir b || framework_failure_
+if ! mv -T --exchange a b 2>exchange_err; then
+  grep 'not supported' exchange_err || { cat exchange_err; fail=1; }
+else
+  test -d a || fail=1
+  test -f b || fail=1
+fi
+
+# Test wrong number of arguments.
+touch c || framework_failure_
+returns_ 1 mv --exchange a 2>/dev/null || fail=1
+returns_ 1 mv --exchange a b c 2>/dev/null || fail=1
+
+# Both files must exist.
+returns_ 1 mv --exchange a d 2>/dev/null || fail=1
+
+Exit $fail
-- 
2.40.1


[Index of Archives]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux