Re: [PATCH 1/2] copy-file-range: new module

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 1344 bytes --]

Although I agree with both of Bruno's reasons, on further thought it makes sense 
for Gnulib to provide a stub instead of an emulation for this particular function.

The primary use for copy_file_range is to copy files more efficiently than a 
read+write loop. However, the read+write loop will typically still be needed 
anyway, to handle non-regular files, copying across file system boundaries, or 
even copying regular input files whose size shrinks while copying as 
copy_file_range (mistakenly, in my view) reports an error if asked to copy past 
end of file.

Because of all these special cases, a reliable use of copy_file_ranges will need 
to be followed by a read+write loop that handles the special cases where 
copy_file_ranges fails. And this read+write loop will work even if 
copy_file_range is a stub that always fails. So there's no practical need to 
emulate copy_file_range.

For an example of the code I'm thinking of, see the call to copy_file_range that 
I recently introduced into GNU Emacs:

https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=486a81f387bb59b2fbbc6aff7b41adbe1621394e

Because of the above, I simplified Gnulib's implementation of copy_file_range by 
installing the attached. If we find a good use for copy_file_range that could 
take advantage of an emulator rather than a stub, we can revert this.

[-- Attachment #2: 0001-copy-file-range-simplify-into-a-stub.txt --]
[-- Type: text/plain, Size: 9275 bytes --]

From b0d998d2e04050dd3c65816b277cf0f91c9ca74c Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 6 Jun 2019 15:44:33 -0700
Subject: [PATCH 1/2] copy-file-range: simplify into a stub
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Based on a comment by Florian Weimer in:
https://lists.gnu.org/r/bug-gnulib/2019-06/msg00007.html
It turns out that Emacs (which will use this module) won’t need an
emulation and I suspect other programs won’t either, because these
programs will need to fall back on read+write anyway.  Perhaps I
am wrong and other programs will be able to use an emulation; if
so, this patch can be reverted.
* lib/copy-file-range.c (COPY_FILE_RANGE): Replace with a stub.
Just call it copy_file_range.
* m4/copy-file-range.m4 (gl_FUNC_COPY_FILE_RANGE):
Check via AC_LINK_IFELSE.
* modules/copy-file-range (Depends-on): Remove modules no longer used.
---
 ChangeLog               |  16 +++++
 lib/copy-file-range.c   | 152 +++-------------------------------------
 m4/copy-file-range.m4   |  22 +++++-
 modules/copy-file-range |   5 --
 4 files changed, 46 insertions(+), 149 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index ac9a44652..5e51f2dbd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,19 @@
+2019-06-06  Paul Eggert  <eggert@cs.ucla.edu>
+
+	copy-file-range: simplify into a stub
+	Based on a comment by Florian Weimer in:
+	https://lists.gnu.org/r/bug-gnulib/2019-06/msg00007.html
+	It turns out that Emacs (which will use this module) won’t need an
+	emulation and I suspect other programs won’t either, because these
+	programs will need to fall back on read+write anyway.  Perhaps I
+	am wrong and other programs will be able to use an emulation; if
+	so, this patch can be reverted.
+	* lib/copy-file-range.c (COPY_FILE_RANGE): Replace with a stub.
+	Just call it copy_file_range.
+	* m4/copy-file-range.m4 (gl_FUNC_COPY_FILE_RANGE):
+	Check via AC_LINK_IFELSE.
+	* modules/copy-file-range (Depends-on): Remove modules no longer used.
+
 2019-06-04  Paul Eggert  <eggert@cs.ucla.edu>
 
 	copy-file: prefer copy_file_range
diff --git a/lib/copy-file-range.c b/lib/copy-file-range.c
index a1448d0d9..39b5efbc1 100644
--- a/lib/copy-file-range.c
+++ b/lib/copy-file-range.c
@@ -1,5 +1,5 @@
-/* Emulation of copy_file_range.
-   Copyright 2017-2019 Free Software Foundation, Inc.
+/* Stub for copy_file_range
+   Copyright 2019 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
@@ -14,152 +14,20 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
 
-/* This file is adapted from glibc io/copy_file_range-compat.c, with a
-   small number of changes to port to non-glibc platforms.  */
+#include <config.h>
 
-#include <libc-config.h>
-
-/* The following macros should be defined:
-
-   COPY_FILE_RANGE_DECL   Declaration specifiers for the function below.
-   COPY_FILE_RANGE        Name of the function to define.  */
-
-#define COPY_FILE_RANGE_DECL
-#define COPY_FILE_RANGE copy_file_range
+#include <unistd.h>
 
 #include <errno.h>
-#include <fcntl.h>
-#include <inttypes.h>
-#include <limits.h>
-#include <sys/stat.h>
-#include <sys/types.h>
-#include <unistd.h>
 
-COPY_FILE_RANGE_DECL
 ssize_t
-COPY_FILE_RANGE (int infd, off_t *pinoff,
+copy_file_range (int infd, off_t *pinoff,
                  int outfd, off_t *poutoff,
                  size_t length, unsigned int flags)
 {
-  if (flags != 0)
-    {
-      __set_errno (EINVAL);
-      return -1;
-    }
-
-  {
-    struct stat instat;
-    struct stat outstat;
-    if (fstat (infd, &instat) != 0 || fstat (outfd, &outstat) != 0)
-      return -1;
-    if (S_ISDIR (instat.st_mode) || S_ISDIR (outstat.st_mode))
-      {
-        __set_errno (EISDIR);
-        return -1;
-      }
-    if (!S_ISREG (instat.st_mode) || !S_ISREG (outstat.st_mode))
-      {
-        /* We need a regular input file so that the we can seek
-           backwards in case of a write failure.  */
-        __set_errno (EINVAL);
-        return -1;
-      }
-    if (instat.st_dev != outstat.st_dev)
-      {
-        /* Cross-device copies are not supported.  */
-        __set_errno (EXDEV);
-        return -1;
-      }
-  }
-
-  /* The output descriptor must not have O_APPEND set.  */
-  {
-    int flags = fcntl (outfd, F_GETFL);
-    if (flags & O_APPEND)
-      {
-        __set_errno (EBADF);
-        return -1;
-      }
-  }
-
-  /* Avoid an overflow in the result.  */
-  if (length > SSIZE_MAX)
-    length = SSIZE_MAX;
-
-  /* Main copying loop.  The buffer size is arbitrary and is a
-     trade-off between stack size consumption, cache usage, and
-     amortization of system call overhead.  */
-  size_t copied = 0;
-  char buf[8192];
-  while (length > 0)
-    {
-      size_t to_read = length;
-      if (to_read > sizeof (buf))
-        to_read = sizeof (buf);
-
-      /* Fill the buffer.  */
-      ssize_t read_count;
-      if (pinoff == NULL)
-        read_count = read (infd, buf, to_read);
-      else
-        read_count = pread (infd, buf, to_read, *pinoff);
-      if (read_count == 0)
-        /* End of file reached prematurely.  */
-        return copied;
-      if (read_count < 0)
-        {
-          if (copied > 0)
-            /* Report the number of bytes copied so far.  */
-            return copied;
-          return -1;
-        }
-      if (pinoff != NULL)
-        *pinoff += read_count;
-
-      /* Write the buffer part which was read to the destination.  */
-      char *end = buf + read_count;
-      for (char *p = buf; p < end; )
-        {
-          ssize_t write_count;
-          if (poutoff == NULL)
-            write_count = write (outfd, p, end - p);
-          else
-            write_count = pwrite (outfd, p, end - p, *poutoff);
-          if (write_count < 0)
-            {
-              /* Adjust the input read position to match what we have
-                 written, so that the caller can pick up after the
-                 error.  */
-              size_t written = p - buf;
-              /* NB: This needs to be signed so that we can form the
-                 negative value below.  */
-              ssize_t overread = read_count - written;
-              if (pinoff == NULL)
-                {
-                  if (overread > 0)
-                    {
-                      /* We are on an error recovery path, so we
-                         cannot deal with failure here.  */
-                      int save_errno = errno;
-                      (void) lseek (infd, -overread, SEEK_CUR);
-                      __set_errno (save_errno);
-                    }
-                }
-              else /* pinoff != NULL */
-                *pinoff -= overread;
-
-              if (copied + written > 0)
-                /* Report the number of bytes copied so far.  */
-                return copied + written;
-              return -1;
-            }
-          p += write_count;
-          if (poutoff != NULL)
-            *poutoff += write_count;
-        } /* Write loop.  */
-
-      copied += read_count;
-      length -= read_count;
-    }
-  return copied;
+  /* There is little need to emulate copy_file_range with read+write,
+     since programs that use copy_file_range must fall back on
+     read+write anyway.  */
+  errno = ENOSYS;
+  return -1;
 }
diff --git a/m4/copy-file-range.m4 b/m4/copy-file-range.m4
index edd2c4aed..20fd05697 100644
--- a/m4/copy-file-range.m4
+++ b/m4/copy-file-range.m4
@@ -11,8 +11,26 @@ AC_DEFUN([gl_FUNC_COPY_FILE_RANGE],
   dnl Persuade glibc <unistd.h> to declare copy_file_range.
   AC_REQUIRE([AC_USE_SYSTEM_EXTENSIONS])
 
-  AC_CHECK_FUNCS_ONCE([copy_file_range])
-  if test $ac_cv_func_copy_file_range != yes; then
+  dnl Use AC_LINK_IFELSE, rather than AC_CHECK_FUNCS or a variant,
+  dnl since we don't want AC_CHECK_FUNCS's checks for glibc stubs.
+  dnl Programs that use copy_file_range must fall back on read+write
+  dnl anyway, and there's little point to substituting the Gnulib stub
+  dnl for a glibc stub.
+  AC_CACHE_CHECK([for copy_file_range], [gl_cv_func_copy_file_range],
+    [AC_LINK_IFELSE(
+       [AC_LANG_PROGRAM(
+          [[#include <unistd.h>
+          ]],
+          [[ssize_t (*func) (int, off_t *, int, off_t, size_t, unsigned)
+              = copy_file_range;
+            return func (0, 0, 0, 0, 0, 0) & 127;
+          ]])
+       ],
+       [gl_cv_func_copy_file_range=yes],
+       [gl_cv_func_copy_file_range=no])
+    ])
+
+  if test "$gl_cv_func_copy_file_range" != yes; then
     HAVE_COPY_FILE_RANGE=0
   fi
 ])
diff --git a/modules/copy-file-range b/modules/copy-file-range
index 18742ef3b..42d6e6f31 100644
--- a/modules/copy-file-range
+++ b/modules/copy-file-range
@@ -6,12 +6,7 @@ lib/copy-file-range.c
 m4/copy-file-range.m4
 
 Depends-on:
-c99
-inttypes-incomplete [test $HAVE_COPY_FILE_RANGE = 0]
 largefile
-libc-config         [test $HAVE_COPY_FILE_RANGE = 0]
-pread               [test $HAVE_COPY_FILE_RANGE = 0]
-pwrite              [test $HAVE_COPY_FILE_RANGE = 0]
 unistd
 
 configure.ac:
-- 
2.17.1


[-- Attachment #3: 0002-copy-file-fix-typo.txt --]
[-- Type: text/plain, Size: 1067 bytes --]

From 30b6215942ffadd705f7641c0725e425057b73a5 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 6 Jun 2019 15:45:14 -0700
Subject: [PATCH 2/2] copy-file: fix typo

* lib/copy-file.c (qcopy_file_preserving): Remove unused label.
---
 ChangeLog       | 3 +++
 lib/copy-file.c | 1 -
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 5e51f2dbd..cbb95d2f5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,8 @@
 2019-06-06  Paul Eggert  <eggert@cs.ucla.edu>
 
+	copy-file: fix typo
+	* lib/copy-file.c (qcopy_file_preserving): Remove unused label.
+
 	copy-file-range: simplify into a stub
 	Based on a comment by Florian Weimer in:
 	https://lists.gnu.org/r/bug-gnulib/2019-06/msg00007.html
diff --git a/lib/copy-file.c b/lib/copy-file.c
index 092a040d7..91c920c33 100644
--- a/lib/copy-file.c
+++ b/lib/copy-file.c
@@ -175,7 +175,6 @@ qcopy_file_preserving (const char *src_filename, const char *dest_filename)
   close (dest_fd);
  error_src:
   close (src_fd);
- error:
   return err;
 }
 
-- 
2.17.1