BIND 10 #3315: IfaceMgr should allow multiple control sockets

[BIND 10 Development]

#3315: IfaceMgr should allow multiple control sockets
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                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:  4             |  Medium
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  4
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * hours:  0 => 4
 * owner:  tmark => tomek
 * totalhours:  0 => 4


Comment:

 First thanks for doing this and so quickly.
 ----------------------------------------------------------------------

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

 it could be satisfied with functions, static class methods, instance
 methods, or even function objects. Like this:

 {{{
 #include <boost/function.hpp>
 #include <boost/bind.hpp>

 typedef boost::function<void ()> ExternalHandler;

 struct SocketCallback {
     int socket_;
     ExternalHandler handler_;
 };

 // Some class that has methods we can use.
 class Buster {
 public:
     void move() {
         std::cout << "moving in the instance" << std::endl;
     }

     static void shake() {
         std::cout << "I'm shaking with static" << std::endl;
     }
 };

 // Plain old function.
 void regFunc() {
     std::cout << "just a regular function, yo" << std::endl;
 }

 // Function object.
 struct func_handler
 {
     void operator()() {
         std::cout << "ain't nothin but an object" << std::endl;
     }
 };

 int main()
 {
     Buster yokel;
     SocketCallback cb;

     // Set to a regular function.
     cb.handler_ = ®Func;
     cb.handler_();

     // Bind it to an instance method
     cb.handler_  = boost::bind(&Buster::move, &yokel);
     cb.handler_();

     // Set it a class method
     cb.handler_ = &Buster::shake;
     cb.handler_();

     // Set it to a function object
     func_handler funky;
     cb.handler_ = funky;
     cb.handler_();
 }
 }}}

 This would provide a great deal more flexibility.  Besides, the typedef
 shouldn't be called a !SessionCallback anymore so you'll be changing it
 anyway (see the following comment) ;)

 --------------------------------------------------------------------------------------------
 I think you should  scrub the use of the words "Session" and "session".

 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?

 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.

 -------------------------------------------------------------------------------------

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

 The reality is that the callbacks could do most anything, they may or may
 not have anything to
 do with ASIO-based IO.

 -------------------------------------------------------------------------------------

 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.

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

 Same thing can be found IfaceMgr::receive6()

 -------------------------------------------------------------------------------------
 iface_mgr_unittests.cc

 line 2608   "signle" should be "single"

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

 -------------------------------------------------------------------------------------
 iface_mgr_unittests.cc

 Shouldn't you repeat these tests for receive6?

 -------------------------------------------------------------------------------------

 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."/>
 }}}


 I did not run valgrind on this.

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