On 2023-01-18 03:04, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:
>
>> From: Matthew John Cheetham <mjcheetham@xxxxxxxxxxx>
>> [...]
>> +enum worker_result {
>> + /*
>> + * Operation successful.
>> + * Caller *might* keep the socket open and allow keep-alive.
>> + */
>> + WR_OK = 0,
>> [...]
>> + enum worker_result wr = WR_OK;
>> +
>> + if (client_addr)
>> + loginfo("Connection from %s:%s", client_addr, client_port);
>> +
>> + set_keep_alive(0, logerror);
>> +
>> + while (1) {
>> + if (write_in_full(STDOUT_FILENO, response, strlen(response)) < 0) {
>> + logerror("unable to write response");
>> + wr = WR_IO_ERROR;
>> + }
>> +
>> + if (wr != WR_OK)
>> + break;
>> + }
>> +
>> + close(STDIN_FILENO);
>> + close(STDOUT_FILENO);
>> +
>> + return !!(wr & WR_IO_ERROR);
>> +}
>
> We have cases where we assign "0" to a bitfield-looking structure like
> this, but only in cases where we're planning to use it as a boolean too.
>
> Or, in other cases where we want some to be explicitly <-1.
>
> Here though we're adding a mixed "OK" and error use, which seems a bit
> odd. Shouldn't we pick one or the other?
You make a fair point about bitfields vs simple integer values. This was a
holdover from previous early hacking on this work where I had the bitfield
serve as a way to communicate the aspects of "does this count as an error?"
and "should we close the connection?".
Upon second thought, I think just simple integer values would be fine as
really only an "OK" and "HANGUP" are non-errors (the latter being the case
that the client gracefully ended the connection without an error and we
should exit).
Check for my next iteration for a rework on these `worker_result` values.
> So far (maybe in later commits?) nothing uses WR_HANGUP, and oddly we
> also use the bitfield-looking thing as a return value from main()....
We don't use the `enum worker_result` values as a return from `main`. We only
ever return 0 or 1 as we `return worker()` from `main`, and the only `return`
from `worker()` is `!!(wr & WR_IO_ERROR)` - 1 if we have `WR_IO_ERROR` set,
otherwise 0.
Thanks,
Matthew
