On 11 January 2012 00:28, Bruno Haible <br...@clisp.org> wrote:
>
> - But actually the function copy_file_preserving_error is equivalent to
> _copy_file_preserving. So why not just rename _copy_file_preserving
> to copy_file_preserving_error and ditch the one-line wrapper?
Done.
> - About the naming of the function. We already have two conventions for
> a new function that does not signal an error:
> - In set-mode-acl.c the prefix 'q' (for "quiet").
> - In gl_xoset.h the prefix 'nx_' (as an antagonism to the prefix 'x').
> It would be good if you could adhere to one of these conventions instead
> of inventing a third one.
Done.
> - In the 'case GL_COPY_ERR_ACL' the function copy_acl() has already
> emitted an error message. What you want, I think, is to call qcopy_acl()
> instead of copy_acl. But qcopy_acl is not yet public. Let me address this
> in a separate patch.
Aha, that was the reason for the lack of error message in this case. I
probably knew that when I originally wrote the patch, but forgot, and
just followed Jim's earlier instruction to add an error message.
>> Is there no way to deal with error messages normally, i.e. via
>> gnulib's strerror? Then the error-returning copy_file_preserving could
>> replace the aborting version, and users who want to could check the
>> return code and issue errors in the usual way.
>
> This would be a regression on the quality of the messages. There is no
> errno code for "cannot open backup file for writing".
That's why I said "gnulib's strerror": by using a gnulib function it's
possible to add new error messages.
> <errno.h> contains shorthands for the most commonly encountered error
> cases. It is absolutely normal that more specialized functions have to
> invent their own error codes.
And hence it would be nice to have a strerror that can cope with these.
> Finally, in the unit test,
>
> - Please use ASSERT (from macros.h) instead of assert (from <assert.h>).
Done.
> (Remember that assert() is eliminated if NDEBUG is not defined.)
Surely you mean "if NDEBUG is defined"? Although why would one be
running the tests with NDEBUG defined?
> - For structuring the test code, better move all the code that tests
> copy_file_preserving() into a function of its own, and the new code
> that tests copy_file_preserving_error() also into a function of its own.
> Like it is done in test-regex-quote.c or test-xvasprintf.c.
Done. Thanks very much for the review. New patch attached.
--
http://rrt.sc3d.org
From 91c6a9a9a19062c974dec3b28ccfd4975b524042 Mon Sep 17 00:00:00 2001
From: Reuben Thomas <r...@sc3d.org>
Date: Tue, 10 Jan 2012 19:45:12 +0000
Subject: [PATCH] copy-file: add error-code-returning version.
* lib/copy-file.c: Add code.
* lib/copy-file.h: Add qcopy_file_preserving and return codes.
* tests/test-copy-file.c: Add tests.
---
ChangeLog | 8 ++++
lib/copy-file.c | 102 +++++++++++++++++++++++++++++++++++++++++-------
lib/copy-file.h | 13 ++++++
tests/test-copy-file.c | 25 +++++++++++-
4 files changed, 133 insertions(+), 15 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index f302d88..bf28e46 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -13,6 +13,14 @@
2012-01-10 Reuben Thomas <r...@sc3d.org>
+ copy-file: add error-code-returning version.
+ * lib/copy-file.c: Add code.
+ * lib/copy-file.h: Add copy_file_preserving_error and return
+ codes.
+ * tests/test-copy-file.c: Add tests.
+
+2012-01-10 Reuben Thomas <r...@sc3d.org>
+
users.txt: order package names lexicographically.
* users.txt: Order package names lexicographically.
diff --git a/lib/copy-file.c b/lib/copy-file.c
index 8e79091..bece943 100644
--- a/lib/copy-file.c
+++ b/lib/copy-file.c
@@ -53,47 +53,70 @@
enum { IO_SIZE = 32 * 1024 };
-void
-copy_file_preserving (const char *src_filename, const char *dest_filename)
+
+int
+qcopy_file_preserving (const char *src_filename, const char *dest_filename)
{
int src_fd;
struct stat statbuf;
int mode;
int dest_fd;
+ int err = 0;
char *buf = xmalloc (IO_SIZE);
src_fd = open (src_filename, O_RDONLY | O_BINARY);
- if (src_fd < 0 || fstat (src_fd, &statbuf) < 0)
- error (EXIT_FAILURE, errno, _("error while opening \"%s\" for reading"),
- src_filename);
+ if (src_fd < 0)
+ {
+ err = GL_COPY_ERR_OPEN_READ;
+ goto error;
+ }
+ if (fstat (src_fd, &statbuf) < 0)
+ {
+ err = GL_COPY_ERR_OPEN_READ;
+ goto error_src;
+ }
mode = statbuf.st_mode & 07777;
dest_fd = open (dest_filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0600);
if (dest_fd < 0)
- error (EXIT_FAILURE, errno, _("cannot open backup file \"%s\" for writing"),
- dest_filename);
+ {
+ err = GL_COPY_ERR_OPEN_BACKUP_WRITE;
+ goto error_src;
+ }
/* Copy the file contents. */
for (;;)
{
size_t n_read = safe_read (src_fd, buf, IO_SIZE);
if (n_read == SAFE_READ_ERROR)
- error (EXIT_FAILURE, errno, _("error reading \"%s\""), src_filename);
+ {
+ err = GL_COPY_ERR_READ;
+ goto error_src_dest;
+ }
if (n_read == 0)
break;
if (full_write (dest_fd, buf, n_read) < n_read)
- error (EXIT_FAILURE, errno, _("error writing \"%s\""), dest_filename);
+ {
+ err = GL_COPY_ERR_WRITE;
+ goto error_src_dest;
+ }
}
free (buf);
#if !USE_ACL
if (close (dest_fd) < 0)
- error (EXIT_FAILURE, errno, _("error writing \"%s\""), dest_filename);
+ {
+ err = GL_COPY_ERR_WRITE;
+ goto error_src;
+ }
if (close (src_fd) < 0)
- error (EXIT_FAILURE, errno, _("error after reading \"%s\""), src_filename);
+ {
+ err = GL_COPY_ERR_AFTER_READ;
+ goto error;
+ }
#endif
/* Preserve the access and modification times. */
@@ -123,15 +146,66 @@ copy_file_preserving (const char *src_filename, const char *dest_filename)
/* Preserve the access permissions. */
#if USE_ACL
if (copy_acl (src_filename, src_fd, dest_filename, dest_fd, mode))
- exit (EXIT_FAILURE);
+ {
+ err = GL_COPY_ERR_ACL;
+ goto error_src_dest;
+ }
#else
chmod (dest_filename, mode);
#endif
#if USE_ACL
if (close (dest_fd) < 0)
- error (EXIT_FAILURE, errno, _("error writing \"%s\""), dest_filename);
+ {
+ err = GL_COPY_ERR_WRITE;
+ goto error_src;
+ }
if (close (src_fd) < 0)
- error (EXIT_FAILURE, errno, _("error after reading \"%s\""), src_filename);
+ {
+ err = GL_COPY_ERR_AFTER_READ;
+ goto error;
+ }
#endif
+
+ return 0;
+
+ error_src_dest:
+ close (dest_fd);
+ error_src:
+ close (src_fd);
+ error:
+ return err;
+}
+
+void
+copy_file_preserving (const char *src_filename, const char *dest_filename)
+{
+ switch (qcopy_file_preserving (src_filename, dest_filename))
+ {
+ case 0:
+ return;
+
+ case GL_COPY_ERR_OPEN_READ:
+ error (EXIT_FAILURE, errno, _("error while opening \"%s\" for reading"),
+ src_filename);
+
+ case GL_COPY_ERR_OPEN_BACKUP_WRITE:
+ error (EXIT_FAILURE, errno, _("cannot open backup file \"%s\" for writing"),
+ dest_filename);
+
+ case GL_COPY_ERR_READ:
+ error (EXIT_FAILURE, errno, _("error reading \"%s\""), src_filename);
+
+ case GL_COPY_ERR_WRITE:
+ error (EXIT_FAILURE, errno, _("error writing \"%s\""), dest_filename);
+
+ case GL_COPY_ERR_AFTER_READ:
+ error (EXIT_FAILURE, errno, _("error after reading \"%s\""), src_filename);
+
+ case GL_COPY_ERR_ACL:
+ error (EXIT_FAILURE, errno, _("error copying the ACL to \"%s\""), dest_filename);
+
+ default:
+ exit (EXIT_FAILURE);
+ }
}
diff --git a/lib/copy-file.h b/lib/copy-file.h
index b28ea37..b577d01 100644
--- a/lib/copy-file.h
+++ b/lib/copy-file.h
@@ -28,6 +28,19 @@ extern "C" {
Exit upon failure. */
extern void copy_file_preserving (const char *src_filename, const char *dest_filename);
+/* Error-returning version of copy_file_preserving. */
+extern int copy_file_preserving_error (const char *src_filename, const char *dest_filename);
+
+/* Error codes returned by copy_file_preserving_error. */
+enum {
+ GL_COPY_ERR_OPEN_READ = -1,
+ GL_COPY_ERR_OPEN_BACKUP_WRITE = -2,
+ GL_COPY_ERR_READ = -3,
+ GL_COPY_ERR_WRITE = -4,
+ GL_COPY_ERR_AFTER_READ = -5,
+ GL_COPY_ERR_ACL = -6
+};
+
#ifdef __cplusplus
}
diff --git a/tests/test-copy-file.c b/tests/test-copy-file.c
index 23c7408..894569b 100644
--- a/tests/test-copy-file.c
+++ b/tests/test-copy-file.c
@@ -20,9 +20,31 @@
#include "copy-file.h"
+#include <sys/stat.h>
+#include <stdio.h>
+
#include "progname.h"
#include "macros.h"
+
+static void
+test_copy_file_preserving (const char *file1, const char *file2)
+{
+ copy_file_preserving (file1, file2);
+}
+
+static void
+test_qcopy_file_preserving (const char *file1, const char *file2)
+{
+ ASSERT (chmod (file2, 0400) == 0);
+ ASSERT (qcopy_file_preserving (file1, file2) == GL_COPY_ERR_OPEN_BACKUP_WRITE);
+ ASSERT (remove (file2) == 0);
+ ASSERT (qcopy_file_preserving (file2, file1) == GL_COPY_ERR_OPEN_READ);
+ ASSERT (qcopy_file_preserving (file1, "/dev/full") == GL_COPY_ERR_WRITE);
+ /* FIXME: The following return codes have yet to be tested:
+ GL_COPY_ERR_READ, GL_COPY_ERR_AFTER_READ, GL_COPY_ERR_ACL */
+}
+
int
main (int argc, char *argv[])
{
@@ -36,7 +58,8 @@ main (int argc, char *argv[])
file1 = argv[1];
file2 = argv[2];
- copy_file_preserving (file1, file2);
+ test_copy_file_preserving (file1, file2);
+ test_qcopy_file_preserving (file1, file2);
return 0;
}
--
1.7.5.4
