[RFC][PATCH] krb5 => libverto main loop

Greg Hudson ghudson at MIT.EDU
Wed Aug 17 19:47:20 EDT 2011


Here are the specific issues I found:

* process_routing_update always reconfigures the network.  Consider
creating a helper to decide if an rtm requires reconfig, and eliminating
the goto.

* process_tcp_connection_read always kills the connection after reading
the length.  The original code has a barrier return before the
kill_tcp_connection label and the new code does not.

* The code inconsistently checks whether verto_get_private(ev) returns
null.  Just as examples, free_socket checks, process_packet asserts, and
accept_tcp_connection doesn't check at all.  I prefer not checking at
all since we always set the private data pointer.

* The "Finished sending" comment is now confusingly positioned before a
conditional which checks if we are *not* finished sending, and returns
if so.

* Just as a point of information, at some point the code should change
to not set up the routing socket if IPv4 and IPv6 pktinfo are both
available.  This is slightly complicated by the new design where
loop_setup_routing_socket() is a separate call from
loop_setup_network().  I expect we'll just move udp_flags from a field
of struct socksetup to a pair of global booleans.

Reading this code also raised a couple of issues about libverto's API:

* verto_set_private() has a return value, but can only return false on
badly-formed input (a null event pointer).  This is confusing to both
human and computer analysis, and leads to arguably bad patterns in
calling code like the assert(verto_set_private(...)) in net-server.c.  I
would prefer if this function did not have a return value.

* In net-server.c you use a 0-second timeout to emulate an idle event.
Could this emulation be done inside libverto for back ends which don't
support idle events?

I'll reiterate the style issues I ran into while reviewing the patch.  I
mentioned all but the first of these by voice on Tuesday, but I spoke
kind of quickly at that point:

* Comments should be punctuated complete sentences (exceptions may apply
for short in-line comments which are noun phrases).
* Lines should not be longer than 79 columns.
* Spaces around binary operators always (except . and ->).
* No spaces before for-loop semicolons.

Let me know when you have another revision of the patch to review.





More information about the krbdev mailing list