[bind10-dev] Comments on proposed hooks framework

[Tomek Mrugalski]

On 04.05.2013 18:39, Stephen Morris wrote:
> The first draft of the hooks framework design can be found at:
> http://bind10.isc.org/wiki/Bind10HooksFramework

There's also a list of hooks we envision for DHCP:
http://bind10.isc.org/wiki/DhcpHooks

I had a discussion with Stephen and on our internal list, but since the
topics at hand affect both DNS and DHCP, I thought that it makes sense
to discuss them on bind10-dev.

Here are my comments:

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

Only the first 3 are mandatory, rest are dependent on what the specific
library needs.

The registration would look as follows:
- server calls version() and checks that it is compatible
- server calls libload() - library does whatever initialization it needs
- for a list of supported hooks, server tries to load specific function
name from the library. For example, the first hook for DHCPv6 is
dhcp6_buffer_rcvd(). If the library exports a function with that name,
it is registered for said hook.

That's it. As simple as that. Bear in mind that if we want people to
write hooks, user library must be as easy to implement as possible. This
approach has couple benefits:

- simple to implement for users;
- no mandatory code on the user lib side, except version()
- registration has to be done somewhere, so we will do it on server
side. We hope to implement it correctly. If not, we will be able to fix
it on our own. If users need to write their own registration routines,
they would make some mistakes and new implementers will repeat the same
mistakes over and over again.
- think about a situation in a year or two: there will be many user
libraries, some developed by us, but many more developed by others.
There would be many similar, but slightly different code bases in them
each doing very similar thing. Each registration code base could be
potentially managed by a different organization.

Stephen brought a counter-argument that the registration procedure as
defined now allows extra flexibility, e.g. the server itself can
register some functions. If this is really needed, we could implement
both, starting with the proposed registration (because it is simpler to
implement on server side) and then later implementing both (because it
is simpler for users to use). Another example given was that the library
can register new hooks when needed. That's not a good idea. The library
could modify the list of callbacks while the server is iterating over
it. Nice recipe for problems. We could prepare for that, but we would
have to pay with increased complexity.

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

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. Let me give specific examples. DHCPv6 packet is
received. The first callout is dhcp6_buffer_rcvd(). The next server
operation is parsing that buffer and create option objects. If we skip
it there is no further processing possible. Another example. Once the
packet is parsed, the server calls selectSubnet callout before selecting
subnet on its own. Later address and other configuration parameters are
assigned from that subnet. If we can't select subnet, it is not possible
to further process the packet.

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.

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

5. Calling a callout in the current form does not allow to set const
parameters. They are essential. Let me give you a specific example: when
DHCP server parses a packet, it selects a subnet from which it will
allocate leases and assign other options. Hook will allow the user
library to pick a different subnet. We need to provide a list of
alternatives, so the callout will know what it can choose from. That
information is part of the configuration. It must be read-only at this
stage. If the user library want to update the configuration, it must use
bindctl interface. It must not modify the configuration in the middle of
packet processing.

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

7. removeall() if definitely a bad idea. One library cannot tweak
unregister other library's callouts! If we allow it, we'll get into a
complete mess that will be very hard to debug. Example: user has library
A that is working fine. He then installs library B that removes all
hooks, including those installed by A. Then you'll get a bug report from
this user that library A is broken. For extra points, it could be that
library B will uninstall all hooks in its libunload(). Then even list of
currently loaded libraries won't help you with debugging.

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.

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

10. get/set. We also need a method getNames() or similar to get list of
available parameters. 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).

Tomek