[bind10-dev] Comments on proposed hooks framework

[Stephen Morris]

On 10/05/13 18:57, Tomek Mrugalski wrote:

> 1. register() function I don't think we need register() function at
> all. Mukund said that the registration could be done in libload(),
> but I'd like to go a step further: not require the user library to
> register its hooks at all. It could be done by the server side.
> 
> I was hoping that the library would have the following methods
> exported:
> 
> version() libload() libunload() dhcp6_buffer_rcvd() 
> dhcp6_pkt_parsed() :

There are two points here:

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.

Although Tomek has a point that simply providing functions of the
appropriate name is simpler for the author of the hooks, I don't think
that having a requirement for registration makes it hugely more
difficult.  People who are writing hooks will be programmers and will
be familiar the concept of registration: besides, the only requirement
is for one registration call per function supplied.


2. Use of register()

If we do go for registration, the idea was that version() and register()
would be mandatory, whereas libload()/libunload() were optional.  I
don't mind whether we use this scheme, or require that the registration
functionality is in libload().

On both of these issues, I would appreciate some consensus.  Could I
please have feedback as to which way to go?


> 2. The position parameter. When the user library calls add(), there
> is the last parameter called position. It is intended to specify
> calling order, 0 implies first, 1 implies second etc. It will not
> work. What if there are 2 libraries and both want to be called
> first? What if there is a single library that wants to be called
> 5th? I think we should get rid of that parameter altogether. If
> there are objections and we want to keep it (Michal mentioned an
> example of a wrapper library that wants to call decrypt before and
> encrypt after), then it should be renamed to requested_order and
> have values: FIRST, LAST, DEFAULT (which implies the order in which
> the libraries are loaded).

In his reply to that point
(https://lists.isc.org/pipermail/bind10-dev/2013-May/004618.html),
Michal commented:


> I proposed to include priority, not position. So you would say to want to be
> called very early (0) or very late (1000) or somewhere in the middle. The
> advantage is that you can position relatively against the hooks you already
> know.

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.

If required, we could later add a "guru" interface into the system that
would allow libraries to manipulate the library callback order. (This
would address Michal's example of a wrapper library.)


> 3. return status. Two of return codes will not work: SUCCESS_SKIP
> and COMPLETE_SKIP. Server can't skip certain operations and then
> continue as if nothing happened.
> :
> Fortunately, there's a simple alternative. We already plan to do 
> pre-action and post-action hooks. So if the callout wants to
> override a decision (e.g. select a different subnet), then it can
> set it in the post-action hook.

The idea was the XXX_SKIP would only be valid where it was meaningful
for them to be.  By returning a SKIP code, the callout is signalling
that it has replaced the relevant action f the calling program.  So in
the case of the selectSubnet callout, the hook would only return a SKIP
code if it had selected the subnet itself. (Michal makes this point in
his reply.)

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.


> 4. Server-side registration of hook types. I don't like presented 
> apporach (enum + separate calls to registerHook that essentially 
> register only string-to-enum mapping). It would be better to
> define enum, an then static array of struct {enum type, string
> name}; (that is later used to populate two maps, because we'll
> likely need both type to name and name to type translations).

The idea of using an enum was to address performance issues.  The code
will be full of constructs along the lines of:

if (hook is present) {
   // Incur overhead of setting parameters only if we will use them
   set parameters
   call hook
}

Registration of the available hooks was proposed to catch any typos in
the calling library when they register their callbacks.  This allowed
for the association of an integer with the hook name, and thus the idea
of using the integer to locate the callout when calling a hook rather
than the hook name.  Comparing an integer against limits is cheaper than
a string comparison, so reducing overhead.

Thinking it through, one problem of the proposed approach is that the
hooks for a particular server will all have to be known and defined in
one place.  This might make management a bit awkward if some of the
hooks are located in libraries.  Instead, I now propose to allocate the
hook numbers dynamically, e.g.

int nsas_choose_ns = 0;

void nsas_register_hooks(GlobalHandle& handle) {
   nsas_choose_ns = handle.registerHook("nsas_choose_nameserver");
}

Libraries provide hook registration functions which must be called by
the main program when registering the hooks.  It does mean that the main
program would need to know about the library hook registration
functions, but it wouldn't need as detailed a knowledge of all the hooks.


> 5. Calling a callout in the current form does not allow to set
> const parameters. They are essential.
> :

Use of boost::any allows this.  If the parameters are stored as a
collection of boost::any objects, the "const"ness of the object is
preserved: attempting to cast to the wrong type, or to a non-const
object of the correct type, will cause an exception to be thrown.

(Having say that, there is nothing to prevent the hook using const_cast
to modify the data.)


> 6. remove() parameter "position" does not make sense. See comment
> #2. There are too many possible combinations. We should pick
> remove(const char* hook_name) or remove(CalloutPtr callout).

Accepted.  This is a logical consequence of the change proposed to point
(2).


> 7. removeall() if definitely a bad idea. One library cannot tweak 
> unregister other library's callouts!

With the change proposed in point (2), removeAll() would now only remove
all callbacks on a given hook for the library issuing the call.


> 8. It is confusing what is available to the user library and what
> is part of the server implementation. If we want to continue with
> this registration design, then we should not expose
> GlobalCalloutHandle at all. It should be hidden in server
> implementation. The only API user library allowed to use would be:
> 
> hook_add(CalloutPtr calloutFunction, const char* hook_name, 
> RequestedCallOrder order);
> 
> Where RequestedCallOrder is one of FIRST, LAST, DEFAULT.

Fair point: I suggest GlobalCalloutHandle is split into two classes: one
handling the server-side part of the hooks manipulation, and one the
library side.  The library-side part has the add/remove methods for the
hooks, and set/get methods for library-specific context (but see below
regarding these).


> 9. This design does not specify library registration order. This is
> not very important, but we should specify it for completeness. I
> think the procedure can be as follows: configuration specifies a
> user libraries directory. Libraries are loaded from that directory
> in alphabetic order.
>
> Users then could specify the loading order in similar way as init
> scripts are loaded in /etc/rc?.d/.

We can do that, or we can simply set the library order in the
configuration, the order of the libraries in the configuration being the
order in which they are loaded.


> 10. get/set. We also need a method getNames() or similar to get
> list of available parameters. 

We can do that.

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:

CalloutHandle::parameter(const char* name, T*& param_ptr);

That way, the callout can directly manipulate the objects residing the
in the calling program.  It avoids the need for copy constructors and
assignment operators to be defined for the objects being passed to the
callouts.  If the


> 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.

Stephen