Reuben Thomas wrote: > On 24 July 2011 21:05, Reuben Thomas <r...@sc3d.org> 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"? ...