bug-gnulib
[Top][All Lists]
Advanced

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

Re: copy_file_preserving variant that doesn't exit on error?


From: Reuben Thomas
Subject: Re: copy_file_preserving variant that doesn't exit on error?
Date: Wed, 11 Jan 2012 00:42:45 +0000

On 11 January 2012 00:28, Bruno Haible <address@hidden> 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

Attachment: 0001-copy-file-add-error-code-returning-version.patch
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]