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           |  closed
           Component:  dhcp          |                    Milestone:
            Keywords:                |  Sprint-DHCP-20131016
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |  complete
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * status:  reviewing => closed
 * resolution:   => complete


Comment:

 Replying to [comment:7 tomek]:
 > '''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.
 >
 Done.
 >
 > '''load_unload.cc'''
 > Please add @todo around registry_fname and user_chk_output_fname

 Done.

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

 Done.

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

 Done.

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

 Done.

 >
 > '''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?

 Just a typo.

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

 Duly noted.

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

 Glad it's ok, renaming all this stuff would be a pain.

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

 Initially I was trying to avoid pulling in our libs.  Way past that now.
 Done.


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

 Done.
 The descriptive lines after each message are now 80 or less.  Most of the
 messages themeselves are longer.


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

 I'm on the fence about this.  I think the convention should follow the
 name
 of the library.  If the library name follows a convention, then you're
 golden.

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

 Done.
 Well then blame Stephen, I stole it from his example hook ;)

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

 Oops, I left in a magic number. Done.

 I too thought about comment lines but as you now know, JSON doesn't
 support
 them. I don't know why not but it doesn't.


 > '''/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. ;)

 Gut-n-paste victim again.  Done.

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

 Gone.

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

 Got it.

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

 Created ticket #3209 for this.


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

 Merged changes: git f36aab92c85498f8511fbbe19fad5e3f787aef68

 Post merge, distcheck revealed minor issues regarding logging files and
 unit test data files,
 these were fixed with 4ba8271b4050f3133dd6ca53721c0ce32ec715a3

 Created ChangeLog entry 697.

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