It is always preferable when the person saying
"this code is good code and fine to merge into master" is
distinct from the person who wrote the code. Two sets of eyes
are much better than one.
This change introduced a lot of complexity (and two new goto statements!) into
the already-intricate test_c2s()
and test_s2c()
routines.
For example: The new code depends on socket file
descriptors being strictly increasing integers.
If the final accept()
in the new code fails to return a file descriptor that is a
larger integer than every other in the set, then the select() on line 398 in
test_c2s_srv.c will
have undetermined behavior. It is likely, but definitely
not promised by the OS, that socket file descriptors will be
increasing. This new code introduces the potential for a
subtle bug in long-running servers, because long-running
servers are the ones that are most likely to re-use file
descriptors, and that re-use will break the select().
Also, select()'s
behavior when the last socket created is not the last socket
to be closed and removed from the FD_SET is similarly
undefined, which means some new logic needs to be added near
lines 424-431.
The new code also needlessly breaks websocket support for
the new tests, because the 0 on lines 257 and 258 of test_c2s_srv.c should
be an i.
And that's what I've found so far in my first attempt to
go through the changes in that one file when I was trying to
integrate these changes into my pull request.
-Peter
