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

[Jim Meyering]

Reuben Thomas wrote:
> Ping! I've not heard anything about this; my current patch (as used in
> GNU Zile) still seems to work.

Hi Reuben,

Thanks for the reminder.
A couple of suggestions:

  ERR_READ may be too generic, risking collision with symbols defined by
  other applications.  Perhaps names like GL_COPY_READ_FAILURE,
  GL_ACL_PRESERVE_FAILURE, etc.

Regarding the tests, it'd be nice if your added test code
could exercise a few of the newly-exposed error return values.
I.e., try to copy an unreadable file and assert that you get
the right error code.  Try to copy to a read-only file and
ensure you get a different one.  Try to copy-with-backup in such
a way that creating or writing the backup file fails.

One more thing, I noticed that your ERR_ACL case does not
print anything.  I suppose that is just to remain completely
compatible with existing code?  It seems like it'd be better
to diagnose that failure, too.

Once you have a test module, please repost.

Thanks for doing this.

> On 2 August 2011 18:31, Reuben Thomas <address@hidden> wrote:
>> Here's a revised patch to provide error returning copy-file as well as
>> error-raising copy-file.
>>
>> Of particular interest:
>>
>> 1. What would you like to call the new function?
>>
>> 2. I haven't yet added a test; from looking at the tests for copy-file
>> it seems it should suffice to add a call to remove followed by an
>> asserted call to copy_file_preserving_error (or whatever it's called)
>> after the call to copy_file_preserving, i.e. just repeat the copy with
>> the other version of the function.
>>
>> diff --git a/lib/copy-file.c b/lib/copy-file.c
>> index f9cd9c0..4ae8427 100644
>> --- a/lib/copy-file.c
>> +++ b/lib/copy-file.c
>> @@ -53,47 +53,79 @@
>>
>>  enum { IO_SIZE = 32 * 1024 };
>>
>> -void
>> -copy_file_preserving (const char *src_filename, const char *dest_filename)
>> +enum {
>> +  ERR_OPEN_READ = 1,
>> +  ERR_OPEN_BACKUP_WRITE,
>> +  ERR_READ,
>> +  ERR_WRITE,
>> +  ERR_AFTER_READ,
>> +  ERR_ACL
>> +};
>> +
...
>> +void
>> +copy_file_preserving (const char *src_filename, const char *dest_filename)
>> +{
>> +  switch (_copy_file_preserving (src_filename, dest_filename))
>> +    {
>> +    case 0:
>> +      return;
>> +
>> +    case ERR_OPEN_READ:
>> +      error (EXIT_FAILURE, errno, _("error while opening \"%s\" for 
>> reading"),
>> +             src_filename);
>> +
>> +    case ERR_OPEN_BACKUP_WRITE:
>> +      error (EXIT_FAILURE, errno, _("cannot open backup file \"%s\"
>> for writing"),
>> +             dest_filename);
>> +
>> +    case ERR_READ:
>> +      error (EXIT_FAILURE, errno, _("error reading \"%s\""), src_filename);
>> +
>> +    case ERR_WRITE:
>> +      error (EXIT_FAILURE, errno, _("error writing \"%s\""), dest_filename);
>> +
>> +    case ERR_AFTER_READ:
>> +      error (EXIT_FAILURE, errno, _("error after reading \"%s\""),
>> src_filename);
>> +
>> +    case ERR_ACL:
>> +      exit (EXIT_FAILURE);
>> +
>> +    default:
>> +      exit (EXIT_FAILURE);
>> +    }