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