[bind10-dev] Comments on proposed hooks framework

[Stephen Morris]

On 14/05/13 17:01, Tomek Mrugalski wrote:
> On 13-05-14 07:43, Michal 'vorner' Vaner wrote:
>>> 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?
> We came up with an idea to do pre- and post-action hooks. OVERRIDE 
> status would make the post-action hooks obsolete. I think it is a 
> reasonable trade-off. Nevertheless, OVERRIDE applicability will be 
> defined on a pre-hook basis. In particular, DHCP is unlikely to 
> support it at the early implementation phase.

I don't think OVERRIDE makes post-action hooks obsolete:

pre-action hook: gets control before the action and may completely
replace the server's action.  Returns OVERRIDE to signal to the server
that it doesn't have to do this action.

post-action hook: gets control after the action and inspects what has
been done.  May or may not decide to modify one or more aspects of the
action.

Removing post-action hooks means that the only option when writing a
hook is to completely replace the processing done by the server.  This
would make writing hooks more complicated.

There are another couple of questions here:

Firstly, if OVERRIDE is returned, should the framework call the next
callout in the chain?  I would argue yes, in that the next callout
might be nothing more that something that logs the packet: or it might
do its own processing and want to override/modify what the first
callout did.

A second question is whether the second callout in the chain should be
allowed to inspect and modify the OVERRIDE status returned by the
first.  Again, I think yes, in that it is more flexible this way.  For
example, the second callout might only log packets created by the
first.  This means that the OVERRIDE attribute should be independent
of the status code and is perhaps best something carried by the
CalloutHandle.


>>> 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.
> For what purpose? To use it by some other callout? Adding new 
> parameter would bring in many issues with little benefit. Stephen 
> proposed the parameters to be passed by pointers to avoid copying.
> But who would clean up the pointer that the user library sets? How
> long the pointer is valid?

If a user wants to pass information between the functions on a
per-packet basis, a context is available.  The callout
"context_create" is called before any packet processing starts, and
"context_destroy" is called when packet processing has ended.  The
user library allocates any information it needs in the first call, and
frees it in the last call.  Context is accessible via the reserved
parameter "context".

The calling program can go some way to mitigating mistakes by
destroying the CalloutHandle and recreating it between packets: if
context is stored as a smart pointer to a block of memory, it will be
automatically freed when the handle is destroyed.

Having said that, perhaps rather than a using reserved "context"
parameter, an easier way for the user would be a pair of calls:

CalloutHandle::getContext(const char* name, T& param);
CalloutHandle::setContext(const char* name, const T& param);

... which stores name.value pairs in the same way as the callout
parameters, but in a different namespace.  If a user library wished to
create several context objects, it would relieve the author of the
necessity of creating a structure to store them.


> I was thinking that set() can be used by user library to set
> already specified parameter and would be used to things like
> override the outgoing interface name, packet destination address
> etc.

I'll take this point with the next.

>>> 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.
> Agree on both.

The idea behind the suggestion was to try an emulate the semantics of
the original DHCP hooks proposal, where the declaration of a callout
was something like:

int callout(type1& arg1, type2& arg2, ...)

i.e. the callout is passed references to objects.  It can modify the
contents of the object, but not change the object instance in the
caller. (So in this case, there is no concept of a "set" or a "get" -
methods on the objects are used to obtain and alter their contents.)
This syntax also has the benefit that no data is copied.

It is possible to use a set/get interface, with the signature:

CalloutHandle::get(const char* name, T& param);
CalloutHandle::set(const char* name, const T& param);

... which gets rid of the raw pointers (unless "T" itself is a pointer
to something), but there are some drawbacks when smart pointers are used.

If smart pointers are passed as arguments to avoid copying
(significant) amounts of data between the caller and the callout, the
fact that the pointers are copied into the CalloutHandle object by the
caller and copied again when the callout is called means that we must
beware of semantics.  A "set" will not always be necessary if data is
changed.  For example:


typedef boost::shared_ptr<Pkt4> Pkt4Ptr;

Pkt4Ptr p4ptr;

// The following call copies the contents of the Pkt4Ptr object
// in "handle" into p4ptr.  The object in "handle" was itself
// a copy of the Pkt4Ptr object in the calling function.  As
// Pkt4Ptr is a shared object, after the call all three pointers
// are pointing to the same data.
handle.get("inpacket", p4ptr);

// The following will change a value in the pointed-to packet
// and does not require a "set".  If the callout returns after
// this call, the pointer in the calling program is pointing to
// the updated packet.
p4pptr->setSomething(...)

// The following sets the pointer in the callout to point to
// a completely new packet.  After this call, the pointer in
// the handle object and the pointer in the caller still point
// to the old data.
p4ptr.reset(new Pkt4(...));

// The following copies the pointer back into the handle object.
// After this call, both the pointer in the callout and the
// pointer in the handle point to the new data, the pointer in
// the caller is still pointing to the old data.  To get the
// new data, the caller will need to to copy the shared pointer
// out of the handle object.
handle.set(p4ptr);

So in some cases, "set" needs to be called, in others it doesn't
(although it won't do any harm if it is always called). This has the
potential of confusion if the packet is replaced and the call to "set"
omitted.  Or if the user believes that changes to a packet will not
take effect unless "set" is called. It also means that the caller must
always copy the pointers from the handle object after the call to be
sure of picking up changes. (Although this last step could be omitted
by a slightly more complicated implementation - the framework stores
pointers to caller objects in the handle and dereferences them when
calling the callout.)


Stephen