[bind10-dev] Comments on proposed hooks framework

[Stephen Morris]

On 14/05/13 07:43, Michal 'vorner' Vaner wrote:
> Hello
> 
> On Mon, May 13, 2013 at 02:44:32PM +0100, Stephen Morris wrote:
>> 1. Whether to require the user to register callbacks.
>> 
>> Having the user-written library export functions of the
>> appropriate name or requiring the hook library register them are
>> equally valid ways of working.
> 
> I think it makes sense to support both modes somehow. At least in
> the long term, if not from the beginning.

OK, that's doable.  Implement the register() method, then add
something that allows the calling program to register functions as well.


>> On reflection - and for the first implementation at least - I
>> think that it would be best to have per-library lists, so it will
>> be up to a library to register multiple callbacks for a hook
>> point in the correct order.
> 
> Actually, my point was about callbacks of different libraries. You
> don't really need multiple callbacks of the same library for the
> single hook point, if there's nothing foreign in between them. You
> can have one and then call all your sub-callbacks from there if you
> want.

Probably not, but it does add to flexibility.  And there seems no
reason to prohibit it.


> It is the order of libraries that matters.
> 
>> However, I concede that it does introduce some confusion.  I
>> suggest removing the SKIP codes: if a hook can allow the callout
>> to signal "skip" functionality, it can do so via a parameter.
> 
> Could you point me to a hook that does not make sense to allow
> replacing the internal behaviour?
> 
> Maybe, instead of calling it SKIP, call it OVERRIDE or REPLACE? So
> it is clearer what is meant?

The idea was that:

SUCCESS = All OK, return from callback and call hook from next library.
COMPLETE = All OK, return from callback without calling hook from next
library.

SUCCESS_SKIP, COMPLETE_SKIP = As above, but return indication to
calling program that it should skip the next step of processing.

+ve number = Error, return to calling program.

However, looking at this, the status code holds two pieces of
information: instructions to the hook framework as to what to do
regarding calling the next hook in the sequence, and a return code to
the calling program.  There is also the issue of what happens if one
library returns SUCCESS_SKIP and the next SUCCESS: does the "skip" occur?

Separating this information would seem cleaner. So, a slightly
modified proposal is:

1. Callout returns SUCCESS, COMPLETE (or 0, non-zero).  The former
causes the next callout in the chain to be called, the latter causes
an immediate return.

2. The CalloutHandle object includes {get,set}ErrorNo() and
{get,set}ErrorText() methods.  If an error occurs, the callout should
set the error number non-zero and (optionally) the error text to some
string.  The caller can log the error/throw an exception as appropriate.

3. The "skip" is more problematic. Although we can make it a parameter
to the called function, if it is common function, a separate method on
CalloutHandle might be more appropriate ({get,set}Skip()).  The
advantage of this over a return status is that should there be
multiple libraries, libraries later in the chain have a chance of
inspecting the skip status set by an earlier library and optionally
modifying it.

>> Looking back at the interface for the get/set, is set() really
>> needed? I'm thinking that it would be better to pass everything
>> as pointers:
> 
> Yes. Library may want to add a parameter that did not exist before,
> and keep it there for later.

That was the idea of a per-packet (and per-library) context.  Methods
can pass information between themselves this way.  As the copntext
information is kept inside the CalloutHandle object, a get/set is the
proper way to go.

>> CalloutHandle::parameter(const char* name, T*& param_ptr);
> 
> I'm not sure about this interface. I believe it is more common in
> C++ to have get and set separated, than one universal method that
> can be abused to do both. Also, the raw pointer there smells like a
> place to create bugs.

This has been proposed because of speed.  A get/set interface implies
copying parameter information - copying into the CalloutHandle and
then copying it out again.  That will cause problems when passing
complicated structures around as well as preventing the passing to
callouts of objects declared as singletons.

The above interface just passes pointers to the objects in the calling
program.  A programmer writing a library just has to observe the
simple rule: "don't delete objects passed as parameters or something
bad will happen".

> 
>>> We need to have the ability to pass const arguments to user
>>> library (see my previous comment about passing pieces of
>>> configuration for example use case).
>> 
>> See above with regards to boost::any.
> 
> I'm not against boost::any, that sounds like something we may want.
> But, I don't know if we want to allow any type to be passed.

boost::any allows the storage of objects in different types in a
container, but preserves the type of the object.  To extract the
object, you have to cast the boost::any object to the correct type. So
although you could set a parameter to anything and pass it to the
called function, if it is not the type the function is expecting an
exception is thrown.

> My motivation here is being able to write bindings to other
> languages. You need some mechanism to convert/wrap the types to the
> given language (be it python or something else) and for that, you
> probably need to know the list of supported types in advance.

The above method of storing data types does not prohibit this.
However, from what you are saying, when we come to writing the
bindings for other languages, we would need to add type information
about the parameters.

The current proposal includes a "registerHook()" call: at the time we
come to extend the hooks to other languages, we could extend the
framework with a "registerHookParameter" call, e.g.

Hook la_pre = registerHook("ia_lease_assign_pre");
registerHookParameter(la_pre, "query", TYPE_PKT4PTR);
registerHookParameter(la_pre, "reply", TYPE_PKT4PTR);
:

... where the TYPE_XXX values are an enumeration listing the supported
data types.

Stephen