BIND 10 #3186: Implement shared library of hooks for the Comcast Demo

[BIND 10 Development]

#3186: Implement shared library of hooks for the Comcast Demo
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:  tmark
                Type:  task          |                       Status:
            Priority:  low           |  reviewing
           Component:  dhcp          |                    Milestone:
            Keywords:                |  Sprint-DHCP-20131016
           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 tomek):

 * owner:  tomek => tmark


Comment:

 '''Makefile.am'''
 Please add a todo to the following comment:

 # hooks lib could be a configurable switch

 Doxygen has the capability to generate a list of all @todo
 comments. I'm naively hoping that one day we will start working our
 way through that list.


 '''load_unload.cc'''
 Please add @todo around registry_fname and user_chk_output_fname

 I think the file name prefix should match library name, so it should
 be user_chk_something. That will be convenient if we need to use
 other files from other libraries.

 '''subnet_select_co.cc'''
 I assume that the extern "C" is used, because it is needed by the hooks
 API. Please add a comment that explains that.

 subnet4_select(): There should be a check if the configuration has at
 least
 one subnet. If there are no subnets configured at all, the callout
 should just return. The same is true for subnet6_select().

 '''user.h'''
 Why did you use _USER_H, instead of USER_H? If you're afraid of it not
 being unique enough, perhaps USER_CHK_USER_H would be better?

 Please do not use word 'requestors' in class comment. Requestor has a
 special meaning in DHCP context. This is a special purpose client that
 participates in leasequery exchange. It has almost nothing to do with
 a regular client, except using similar packet format in some cases
 (and very different in others, requestors can connect over TCP as
 well).

 I'm not sure if User is the best name. I would use Client instead. But
 it's ok to leave it as it is.

 '''user.cc'''
 decodeHex: It is better to use already existing methods for hex
 conversions. See isc::util::encode::decodeHex. For example use, see
 src/bin/dhcp6/tests/wireshark.cc.

 '''user_chk_messages.mesg'''
 Some lines are really long. Although the message itself should not be
 wrapped, so it is ok for it to go over line, it is easy to wrap its
 description around.

 '''user_chk_log.cc'''
 What is NSAS? Did you download the code from the Internet again, Tom? ;)

 I'm wondering whether it makes sense to use some common prefix for all
 hook libraries, like hook_user_chk. This is an honest question. If you
 think it's a good idea, please change it and add a line somewhere to
 src/lib/hooks/hooks_user.dox to explain the convention.

 '''user_file.h'''
 Please rename ifs_ to file_. Easier to read.

 '''user_file.cc'''
 readNextUser(): please use constant rather than 4096 and move it to .h
 file. Also mention somewher in the .h file that the limitation is 4096
 lines. I was thinking about skipping lines beginning with # as
 comments, but apparently JSON format does not allow comments.

 '''/tests/test_data_files_config.h.in'''
 I see that your time travel research goes smoothly. But let's pretend
 it was all done in 2013. ;)

 '''tests/Makefile.am'''
 Please remove the line 67. It is commented out. If it should stay
 there, please add an explanation comment.

 '''ChangeLog'''
 The proposed changelog looks almost ok, but please avoid using word
 requestors or requesters. Saying "DHCP clients" is better.

 Please add a ticket for writing documentation for this hook lib. Since
 this is the first one that we're shipping, it'll have to double as a
 real life example, so it needs user documentation. Of course we don't
 have time for such things right now, so a new ticket is the way go.

 That code builds and all unit-tests pass on Ubuntu 13.04 x64.

 If you agree and address all my comments, feel free to merge the code.
 I don't see it again.

-- 
Ticket URL: <http://bind10.isc.org/ticket/3186#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development