[bind10-dev] Comments on proposed hooks framework

Mon May 13 17:34:07 UTC 2013

I think my email never made it to the list,

Here it is again:

Hi Tomek,

Perl's Moose has a very nice feature called: Roles.

I believe BIND10 could benefit from some of the features it provides, for which I'm sure there are C++ equivalent ways of doing them.

Roles allow you to extend an object or module's behavior by inserting code at specific places such as:

* before a method is called
* after a method is called
* around a method call.

"before" and "after" allows you to execute code on those specific points, without affecting the original call, that is, if you had a method call ->example it would run like this

 "before" extension->example(args)
  original->example(args)
 "after" extension->example(args)	

"around" is a bit tricker, it allows you to intercept the original method call and work around it, such as:

 "around" extension->example(args){

	// do some stuff

	// call the original method
       original->example(args);

  }

You can even skip the original call if you want to, as long as the "around" extension returns something useful to the caller.

I find myself using more and more, roles, since it allows me to implement features, without having to go back at the original code and change them.

There is a specific way/policy in which methods are called, and I believe they have to do with the order in which they are registered, if multiple modules register hooks to a specific method, they will be called as a cascade.

There a lot to it, but if you're interested, please read:

 http://search.cpan.org/~ether/Moose-2.0802/lib/Moose/Manual/Roles.pod

Most likely this can be done with C++ Templates or a framework.

Best regards,

Francisco Obispo 
Director of Applications and Services - ISC
email: fobispo at isc.org
Phone: +1 650 423 1374 || INOC-DBA *3557* NOC
PGP KeyID = B38DB1BE

On May 13, 2013, at 6:44 AM, Stephen Morris <stephen at isc.org> wrote:

> 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
> _______________________________________________
> bind10-dev mailing list
> bind10-dev at lists.isc.org
> https://lists.isc.org/mailman/listinfo/bind10-dev

Francisco Obispo 
Director of Applications and Services - ISC
email: fobispo at isc.org
Phone: +1 650 423 1374 || INOC-DBA *3557* NOC
PGP KeyID = B38DB1BE