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

[Jim Meyering]

Reuben Thomas wrote:
> On 24 July 2011 21:05, Reuben Thomas <address@hidden> wrote:
>> I just came across the copy-file module, which does exactly what I
>> want (it is even geared to making backup files), but (unfortunately
>> for use in an interactive program) exits on error.
>
> Just to move this along a bit, I attach a patch which changes
> copy_file_preserving to return error codes instead of calling error.

Hi Reuben,
Thanks for pursuing this.
It would indeed be useful to have code with equivalent
functionality that does not exit upon failure.

However, that "equivalent" means that an improved
version should return enough information so that the
caller can emit the same diagnostics.

Ideally, copy_file_preserving would retain it semantics
and it would simply call your new function (containing the
guts of this one), obtain an indication of which part (if any)
failed, and then diagnose and exit.

As you can see, there will have to be many distinct
error codes specific to this task.  I suspect that
the wrapper copy_file_preserving will use a large
case statement to map each of those codes to its
corresponding diagnostic.

I glanced through your patch and spotted a leak:

...
> diff --git a/lib/copy-file.h b/lib/copy-file.h
...
> -void
> +int
>  copy_file_preserving (const char *src_filename, const char *dest_filename)
>  {
>    int src_fd;
> @@ -63,37 +63,37 @@ copy_file_preserving (const char *src_filename,
> const char *dest_filename)
>    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)
> +    return -1;
> +  if (fstat (src_fd, &statbuf) < 0)
> +    goto error_exit_2;
>
>    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);
> +    return -1;

If we return here, wouldn't we leak "src_fd"?

...