[Swan-dev] typing libevent callback functions

[Antony Antony]

On Sun, Jan 25, 2015 at 04:10:12PM -0500, D. Hugh Redelmeier wrote:
> Getting callback functions right is tricky.  One tool that can detect
> some problems is the C type system.

I see some appeal this idea, however, I don't have a clear picture of what could potentially go wrong otherwise. What kind of error would be caught in the context we are using. I think this could be useful if function pointer is stored in a variable, such as crypto_req_cont_func is used. So far that is not the case for the libevent callbacks. We just pass the function pointer to libevent using pluto_event_new. Any references for further reading?

Having said that, this is a minor change lets do it. One clarification question bellow.

> This approach was used with crypto request continuations.  See how the
> type crypto_req_cont_func is used.
> 
> I think that this technique/convention should be used for libevent
> callbacks.
> 
> It requires a typedef for callback functions.
> 
> Unfortunately, libevent2's event.h declares a type that is called
> event_callback_fn but is really a POINTER to a function:
>   typedef void (*event_callback_fn)(evutil_socket_t, short, void *);
> 
> We have to provide our own typedef:
>   typedef void event_callback_routine(evutil_socket_t fd, short events, void *arg);
> Notice that I've included the normal argument names: I think that it
> makes their meaning clearer.
> 
> We should declare each callback function with this type.  That's how
> to get the compiler to enforce the type matching requirements.  It
> also is a flag to the reader that the function is a callback function.
> 
> extern event_callback_routine do_read_cb;
> 
> Unfortunately you cannot define the functions using this type because
> a function definition needs an explicit parameter list.
> 
> void do_read(evutil_socket_t fd, short events, void *arg) {
> 	/* whatever */
> }
> 
> I suggest that each callback function use the same names for formal
> parameters.  (This is not currently the case.)

good catch. That is great idea.

> 
> Here's a list of the callback functions used with pluto_event_new:
>     kernel_process_msg_cb
>     kernel_process_queue_cb
>     comm_handle_cb
>     handle_helper_answer_cb
>     timer_event_cb

do we need one typedef per function? I am guessing not.
Please take a look at bdf978aa5b441fec27. Is the right direction?

thanks,
-antony