BIND 10 #3315: IfaceMgr should allow multiple control sockets

[BIND 10 Development]

#3315: IfaceMgr should allow multiple control sockets
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  libdhcp       |                    Milestone:  DHCP-
            Keywords:                |  Kea0.9-alpha
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:
         Total Hours:  12            |  Medium
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  8
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * hours:  4 => 8
 * owner:  tomek => tmark
 * totalhours:  4 => 12


Comment:

 Replying to [comment:4 tmark]:
 > First thanks for doing this and so quickly.
 Glad I could help.

 > Currently the callback function member in the !SocketCallback is
 typedef'd as a void function pointer:
 >
 > {{{
 >     typedef void (*SessionCallback) (void);
 > }}}
 >
 > If instead it were  a boost::function<> like this:
 >
 > {{{
 >     typedef boost::function<void ()>ExternalHandler
 > }}}
 Done.

 > I think you should  scrub the use of the words "Session" and "session".
 I hope I picked them all. They are now called external sockets and
 external callbacks.

 > The label "Session" ties directly to BIND10's current architecture and
 specifically to its Session layer.  I realize this permitted the change
 without so much ripple effect, however it is name we will get stuck with
 as it will only get propagated.   IFaceMgr doesn't "know" about BIND10
 right?
 Nope. That could be a stand-alone library.

 > Similarly there are comments, like line 954 in iface_mgr.h:
 >
 > {{{
 >     // if there are any session sockets registered...
 > }}}
 >
 > It should say "external" sockets.  This applies throughout iface_mgr.h
 and .cc and of
 > course the unit tests.
 I hope I picked them all. If not, please let me know where.

 > iface_mgr.cc
 >
 > You could replace the comment block at line 987 with something like
 this:
 >
 > "Calling the external socket's callback provides its service layer
 access
 > without it directly it IfaceMgr."
 Did something similar.

 > iface_mgr.cc
 >
 > IfaceMgr::receive4()
 >
 > Sends list of sockets and their "names" to a local ostreamstring, names.
 Nothing is ever done
 > with this.  It should be removed unless I'm missing something.  I know
 you didn't add this
 > but I don't see a point in keeping it either.
 After a discussion on Jabber, we decided to do that. This looks like a
 work for separate ticket, so I created #3319.

 > If you do keep it, then you should consider adding a label to the
 SocketCallback struct.
 > At least this way if they are logged you can tell what is what.  At the
 very least you
 > need to replace the "(session)" label in use now with "(external)".
 Removed from both receive{4,6}.

 >
 -------------------------------------------------------------------------------------
 > iface_mgr_unittests.cc
 >
 > line 2608   "signle" should be "single"
 Fixed.

 > iface_mgr_unittests.cc
 >
 > TEST_F(IfaceMgrTest, DeleteControlSessions)
 >
 > This test unregisters the first external socket but does not verify that
 it no longer
 > has an affect.  After calling deleteExternalSocket(), you should send
 data over the
 > first pipe and verify that it's callback does NOT get called.
 Test improved.

 BTW the test is now called DeleteExternalSockets4 (and there is
 DeleteExternalSockets6).

 >
 -------------------------------------------------------------------------------------
 > iface_mgr_unittests.cc
 >
 > Shouldn't you repeat these tests for receive6?
 I was trying to be more IPv4 friendly and that's what I get in return? ;)

 Ok, IPv6 tests implemented. Since there are no common ancestor for Pkt4
 and Pkt6, they are separate tests. I will not implement templated tests at
 this time.

 > Although not caused by this change, cppcheck spit these out:
 >
 > {{{
 >     <error
 file="/Users/tmark/build/trac3315/bind10/src/lib/dhcp/iface_mgr.h"
 line="1029" id="passedByValue" severity="style" msg="Function parameter
 'addr' should be passed by reference."/>
 >     <error file="src/lib/dhcp/iface_mgr_bsd.cc" line="155"
 id="passedByValue" severity="style" msg="Function parameter
 'addr' should be passed by reference."/>
 >     <error file="src/lib/dhcp/iface_mgr_linux.cc" line="539"
 id="passedByValue" severity="style" msg="Function parameter
 'addr' should be passed by reference."/>
 >     <error file="src/lib/dhcp/iface_mgr_sun.cc" line="159"
 id="passedByValue" severity="style" msg="Function parameter
 'addr' should be passed by reference."/>
 > }}}
 Done.

 > I did not run valgrind on this.
 Me neither. It compiles, so it much be good, right?

 Seriously speaking, the changes are benign enough that we should be fine.
 If not, the build farm turning red will prove us wrong.

 Adding +4 hours this time and +4 for the initial work.

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