BIND 10 #2995: Add selected few (3-4) hoots into DHCPv6 server
BIND 10 Development
do-not-reply at isc.org
Fri Jun 28 12:19:18 UTC 2013
#2995: Add selected few (3-4) hoots into DHCPv6 server
-------------------------------------+-------------------------------------
Reporter: tomek | Owner: tomek
Type: enhancement | Status:
Priority: medium | reviewing
Component: dhcp6 | Milestone:
Keywords: | Sprint-DHCP-20130703
Sensitive: 0 | Resolution:
Sub-Project: DHCP | CVSS Scoring:
Estimated Difficulty: 0 | Defect Severity: N/A
Total Hours: 0 | Feature Depending on Ticket:
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by stephen):
* owner: stephen => tomek
Comment:
Reviewed commit commit c6b3ce6d16f92b80db77487900fb57a7a67b3203
This is a bit long, mainly because it includes some discussion of the
hooks system related to changes here.
Note that some changes have been made and should be pulled before
development continues.
'''doc/devel/mainpage.dox'''[[BR]]
'''src/bin/dhcp6/dhcp6_hooks.dox'''[[BR]]
I've edited these files and altered the format a bit. In particular:
* The Doxygen tags convention appears to be the same as for method names.
I've renamed the tag "hooks-dhcp6" to "hooksDhcp6". (I note that the
Logging pages don't follow that convention, but an update of them is
scheduled.)
* The description for each hooks has been split, with the setting of the
"skip" flag put in a separate section. (This may need to change if we
alter the skip flag - see later in this comment.)
* I've altered the reference to the general Hooks API document in
anticipating of the merging of #2982.
'''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''[[BR]]
You may want to add the removal of the hook points to the Dhcpv4SrvTest
constructor as well, in anticipation of the time hooks are added to
DHCPv4.
'''src/bin/dhcp6/dhcp6_messages.mes'''[[BR]]
DHCP6_HOOK_SUBNET6_SELECT_SKIP: the general idea is that when the callout
instructs the server to skip the next stage of processing, it is because
the callout has already done that. So in the case of the skipping of
subnet selection, it would be because the callout has already selected the
subnet. (How/why I've no idea - but it would be logical for the selected
subnet to be returned through the argument list somehow. And how the
server translates what is returned into a pointer to the appropriate data
structure is another issue.)
DHCP6_HOOK_PACKET_RCVD_SKIP: As noted above, the "skip" flag was intended
to instruct the server to skip the processing associated with the hook·
In some cases - such as here - there is no associated processing: instead,
it makes sense to allow the callout to drop the packet. I suggest that the
last two sentences of the explanation be replaced with "For this
particular hook, the setting of the flag by a callout instructs the server
to drop the packet." (But see question below.)
DHCP6_HOOK_PACKET_SEND_SKIP: suggest a similar change to the wording as
for DHCP6_HOOK_PACKET_RCVD_SKIP - replace the last two sentences with
something like "For this particular hook, the setting of the flag by a
callout instructs the server to drop the response." (But see below.)
----
'''Question about hooks system''': the "skip" flag was so-named because of
anticpated use. What I hadn't considered was the possibility of dropping
the packet (hence the overloaded use of the skip flag by the server in
this situation). On the assumption that it seem illogical to a hook
author that setting the "skip" flag causes the dropping of the packet, I
think this should be changed. Although we could require that the action
be returned to the server in the argument list, an easily-accessible flag
or value appears to be more convenient (and faster). Would it be better
to replace {get,set}!SkipFlag() with something like
{get,set}!ReturnAction() (where the "return action" value is an "int")?
Appropriate values for the return action indicator would be defined on a
per-hook basis, although adopting the convention that a value of 0 means
that the server should continue normal processing will simplify things.
So for the above hook points, a return value of 0 means continue and a
non-zero value is interpreted as "drop the packet" and the send and
receive points, and "skip the associated processing" at the subnet
selection point. (Some hook points may have more than two possible return
actions (e.g. drop, skip, or continues) which is why it is suggested that
the the "return action" be an integer value rather than a boolean flag.)
----
(Incidentally, if we go for this option, the error codes above would be
better named xxx_DROP instead of xxx_SKIP.)
'''src/bin/dhcp6/dhcp6_srv.h'''[[BR]]
Methods getServerHooks/getHooksManager. These won't be needed after #2980
is merged. (See end of this comment.)
The methods getCalloutHandle(), packetProcess{Start,End} should be
documented.
'''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
With the changes listed at the end of this comment, the include of
server_hooks.h can be removed.
Dhcpv6Srv constructor: unless Dhcpv6Srv is a singleton created before the
configuration file is read and hook libraries loaded, hook registration
should not be done here. For a suggested way of registering hooks when
the program is loaded, see the "Guide to Hooks for the BIND 10 Component
Developer" (in the .dox file) in #2980.
Dhcpv6Srv constructor: the changes in #2980 should mean that there is no
need to call HooksManager::loadLibraries(). If no libraries are loaded
when something is called, the framework will set itself up with an empty
set of libraries (as you have done here).
Dhcpv6Srv destructor: "todo" comment about deregistering a hook. I've been
thinking this through and "deregister()" is not needed. In this case
there is only one Dhcpv6Srv object which is in existence all the time the
server is running, so there is no need to deregister them.
----
In our Jabber conversation a couple of days ago, we discussed hooks in an
object such as the allocation engine, where we might have multiple
allocation engines using different algorithms, and choose one based on a
configuration option. However, even then deregisterHooks is not needed.
There are several reasons for this:
* Although the allocation engine object is create and destroyed when the
configuration is changed, the file is loaded when the program is started.
Therefore the hooks can be registered then.
For the the case where we have an alternate allocation engine available,
then one of the following applies:
* Either or both the main engine and alternate engine contain a hook that
is not in the other engine. In that case, the hook should be uniquely
named and can be declared at start-up. If the particular engine is not
loaded, any callouts attached to that hook will not be called.
* Both engines contain a common hook called at the same (logical) point in
each of them. In this case, since the engines fit in the same place in
the server, they will be derived from a common base class. The hook call
can be encapsulated in a method in the common base class and the hook
point registered when that the file containing the base class is loaded.
- Both engines contain a hook with the same name but at different logical
points in the code (and possibly with different arguments). This causes
problems - when libraries load they identify hooks by name. What happens
if the hooks library for the main engine is loaded when the alternate
engine is running (or vice-versa)? The wrong callout will be called at
that hooks point. We should should not really have any code with
conflicting hook names.
In addition to the above, there is the issue concerning the interaction of
the reconfiguration of the allocation engine with the reloading of the
hooks libraries. Suppose a library containing a callout for a hook that
is only in the alternate engine is loaded when the main engine is the
current engine. The code for that hook won't be registered. If we now
switch to the alternate engine, the hook won't be registered unless we
reload the library. This could be done, bit it is messy - every time we
make a switch of the allocation engine we need to reload all hooks
libraries. (And it potentially applies to every case where we have
alternate engines, e.g. suppose we add hooks to the database backend
code?)
In summary, hook names should be registered on start-up. They are either
unique to a piece of code - in which case there is no problem. Or they are
common to bits of code derived from a common ancestor - in which the code
should call the hook through a method in the common base class. There is
no need to deregister the names.
----
Dhcpv6Srv::run(): "Callouts decided to skip the next processing step".
The comment is incorrect: as indicated the comment (above) for
DHCP6_HOOK_SUBNET6_SELECT_SKIP, callouts can skip a processing step when
it makes sense to do so. In other cases - such as here - "skip" makes no
sense, but "drop" does.
Dhcpv6Srv::run(): "No need to clear arguments". This is true, but the
"deleteAllArguments()" call should be negligible overhead if no arguments
are stored. It might be more defensive to clear them regardless. (On the
other hand, callouts should only reference documented arguments. If they
access something undocumented - such as an argument from a previous call
that has not been deleted - we can't be held responsible if a changes to
BIND 10 causes their code to fail.)
Dhcpv6Srv::run(): Would the names of the arguments to the callouts be more
descriptive if named "query" and "response" rather than "pkt6"?
Dhcp6Srv general: "!CalloutHandlePtr" has been defined and can be used
instead of boost::shared_ptr<!CalloutHandle>.
Dhcp6Srv::selectSubnet(): this has the same comment as the "run()" method
- "This is the first callout so not need to clear any arguments". Both
statements can't be true.
Dhcp6Srv::assignIA_NA(): there is a call to get the !CalloutHandle here
although the return value is not used anywhere.
Dhcpv6Srv::getCalloutHandle(): just a comment here that this scheme is
only valid providing a packet is fully processed (from being received to
the response being sent (or the query dropped)). This is the case for the
server, but comments in the method should make the restrictions clear.
'''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''[[BR]]
This file is now 2,500 lines long - at some time we should break it down
into smaller files.
"Hooks" test: serverHooks::getIndex() will throw a !NoSuchHook exception
if the hook is not registered, so these tests should really be
EXPECT_NO_THROW tests.
HooksDhcpv6SrvTest class: methods should be documented, including
arguments.
HooksDhcpv6SrvTest class: once #2980 is merged, there will be no need to
set the library index in the constructor.
Note: once #2980 is merged, we should think of creating a shared library
containing a variant of a HooksDhcpv6SrvTest object and repeat the hooks
tests here using that library.
'''src/lib/dhcpsrv/alloc_engine.{cc,h}'''[[BR]]
See above for discussion of hook registration questions.
See below for future changes regarding getHooksManager() etc.
AllocEngine::reuseExpiredLease(): there is no need to reset the "skip"
flag - that is automatically set "false" in HooksManager::callCallouts()
before the first callout is called.
AllocEngine::reuseExpiredLease(): (comment before callCallouts()) is this
the first callout as well?
AllocEngine::reuseExpiredLease(): the callout called is for lease6_select,
yet this method is described as reusing an expired lease (and the comment
before the callouts talks about lease6_ia being added). Reusing an
expired lease seems to be an event that merits its own distinct callout.
AllocEngine::createLease6(): the callout called in this method has the
same hook index as that in reuseExpiredLease() (as well as the same
comments, including "this is the first callout". I suspect a cut-and-
paste error somewhere, probably in reuseExpiredLease().
'''src/lib/dhcpsrv/cfgmgr.{cc,h}'''[[BR]]
getSubnets6(): for getter and setter methods that get/return a member of
the class (and that are not virtual), putting the single line of the code
in the header allows the compiler to inline the call and save the overhead
of a call/return.
'''src/lib/dhcpsrv/dhcpsrv_log.h'''[[BR]]
DHCPSRV_HOOKS should be DHCPSRV_DBG_HOOKS to be consistent with other
debug levels.
'''src/lib/dhcpsrv/dhcpsrv_messages.mes'''[[BR]]
DHCPSRV_HOOK_LEASE_IA_ADD_SKIP:[[BR]]
s/callout assigned on lease6_ia.../callout installed on the
lease6_ia.../[[BR]]
s/Server will not.../The server will not.../
'''src/lib/dhcpsrv/tests/alloc_engine_unittest.cc'''[[BR]]
General: The EXPECT_EQ methods should have the expected value as the first
argument and the value under test as the second (Google test messages are
optimised for this order).
See elsewhere about deleting hook names. (Note that tests need to register
them even if they have been registered in the file holding the code being
tested: other tests may have deleted those names.)
various: the calls to loadLibraries will not be required after #2980 is
merged.
= Updates required after #2980 is merged =
After #2980 is merged (assuming it gets through the review unscathed), the
following changes should to be made:
'''1.''' getHooksManager() calls are no longer needed. All !HooksManager
instance methods have been wrapped in static methods that call the
instance manager with the singleton instance. For example, you can
replace:
{{{
HooksManager::getHooksManager().loadLibraries(...);
}}}
with
{{{
HooksManager::loadLibraries(...);
}}}
etc.
'''2.''' getServerHooks() calls are no longer needed except in unit tests.
The main code should only be registering hooks - only the test code should
be clearing the hooks list of hooks. A !HooksManager wrapper around
ServerHooks::registerHook() has been written so that:
{{{
ServerHooks::getServerHooks().registerHook(...);
}}}
can be replaced by a call to the static method:
{{{
HooksManager::registerHook(...);
}}}
In this way, the main code only need access the !HooksManager and
!CalloutHandle classes (and possibly the !LibraryHandle class if the
server registers its own callouts.)
'''3.''' To clear the list of hooks, the unit test code will still need to
call
{{{
ServerHooks::getServerHooks().reset();
}}}
This should be done in the test class constructor. (Putting a call in the
destructor as well does no harm.)
'''4.''' Within the unit tests, register a callout that is defined in the
test file with:
{{{
HooksManager::preCalloutsLibraryHandle().registerCallout(...)
}}}
(There is also a postCalloutsLibraryHandle() method, but unless any shared
libraries are loaded, the pre- and post- methods do effectively the same
thing.)
'''5.''' We should write some tests that involve loading a shared library
while the test is running.
--
Ticket URL: <http://bind10.isc.org/ticket/2995#comment:4>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list