Johannes Sixt <j6t@xxxxxxxx> writes:
> Ilari Liusvaara schrieb:
>> +static inline void force_close(int fd)
>> +{
>> + /*
>> + * The close is deemed success or failed in non-transient way if
>> + * close() suceeds, returns EBADF or error other than EINTR or
>> + * EAGAIN.
>> + */
>> + while (close(fd) < 0 && errno != EBADF)
>> + if(errno != EINTR && errno != EAGAIN)
>> + break;
>
> You are constantly ignoring proposals to iterate only on EINTR and
> EAGAIN, but do not make an argument why you do otherwise. Did I miss
> something?
Meaning something like this?
static inline void force_close(int fd)
{
/*
* Retry failed close() on EINTR or EAGAIN
*/
while (close(fd) < 0 && (errno == EINTR || errno == EAGAIN))
; /* try again */
}
which should be equivalent as long as EBADF is different from EINTR and
EAGAIN, I think.
>> + if (cmd->pid > 0) {
>> + int r = 0, ret;
>> + force_close(report_pipe[1]);
>> +read_again:
>> ...
>> + if(waitpid(cmd->pid, &ret, 0) < 0 && errno == EINTR)
>> + /* Nothing. */ ;
>> + cmd->pid = -1;
>
> As per Documentation/technical/api-run-command.txt, you should write
> an error here, except if (failed_errno==ENOENT &&
> cmd->silent_exec_failure!=0).
I was planning to replace the earlier series that was dropped from pu with
this iteration, but I guess I'll wait for another round before doing so.
Thanks for reviewing.
>> +test_expect_success "reporting ENOENT" \
>> +"test-run-command 1"
>
> I wonder what this parameter "1" is good for...
I guessed that this is for adding more tests to test-run-command in the
future and not having to change this test, in which case I think it is a
sensible thing to do.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html