On Wed, Jul 23, 2025 at 03:28:05AM -0400, Eric Sunshine wrote:
> > close(fd);
>
> The descriptor is closed manually (again) because a subsequent open()
> call is going to reuse the variable. However...
>
> > if (argv[1][1] == 'd')
> > out_buf = diff_delta(from_buf, from_size,
> > data_buf, data_size,
> > &out_size, 0);
> > else
> > out_buf = patch_delta(from_buf, from_size,
> > data_buf, data_size,
> > &out_size);
> > if (!out_buf) {
> > fprintf(stderr, "delta operation failed (returned NULL)\n");
> > goto cleanup;
> > }
>
> ...although `fd` was closed, it still holds the previously-open
> non-negative file descriptor, which means that this `goto cleanup`...
Oof, good catch. This iteration of the patch was based on my suggestion,
but I didn't notice the jump to cleanup between that close/open pair.
I wonder if it would be more clear written as:
int from_fd = -1;
int data_fd = -1;
int out_fd = -1;
...
from_fd = open(...);
if (...errors...)
goto cleanup;
close(from_fd);
from_fd = -1;
[...same for other fds...]
cleanup:
if (from_fd >= 0)
close(from_fd);
[etc]
You could even drop that first close() in the happy path entirely, if
you don't mind holding all three descriptors open at once.
I dunno. We are reaching diminishing returns spending brainpower on a
function that is meant to be somewhat quick-and-dirty.
-Peff
