BIND 10 #3203: Minimalistic client classification is needed for docsis (classification, part 1)

[BIND 10 Development]

#3203: Minimalistic client classification is needed for docsis (classification,
part 1)
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  enhancement   |                       Status:
            Priority:  high          |  reviewing
           Component:  dhcp          |                    Milestone:  DHCP-
            Keywords:                |  Kea1.0-alpha
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  4             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  4
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

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


Comment:

 General:

 1. You are using hard-coded strings for class names. These should be
 defined as constants somewhere.

 2. Pkt4::classes_, inClass(), and addClass() are all identical to Pkt6.
 They
 should be moved to the base class.

 ------------------------------------------------------------------------------
 dhcp/Pkt4.h

 Commentary for ::addClass is conceptually backwards.  It describes adding
 packets to classes when it fact it addes classes to a packet.
 (Same in dhcp/Pkt6.h)

 ------------------------------------------------------------------------------
 dhcp/tests/pkt4_unittests.cc

 For the sake of thoroughess the test should verify that  inClass("foo")
 returns
 true after adding it.  This ensures that no class value specific logic
 exists
 that would interfere.
 (Same in dhcp/tests/pk6_unittests.cc)

 ------------------------------------------------------------------------------
 src/bin/dhcp4/dhcp4_srv.cc:

 Dhcpv4Srv::classifyPacket()

 There a few issues with the if-block beginning at line 1808.

 1. Why do you have this block at all?  Regardless of the value in
 vendor_class
 the net effect is the same:
   a. you add the value to the string "classes"
   b. you call pkt-addClass() passing in the value

 Seems like you could just replace the whole block and subsequent test for
 logging with this:

 {{{
     if (!classes.empty()) {
         classes += vendor_class->getValue();
         pkt->addClass(vendor_class->getValue());
         LOG_DEBUG(dhcp4_logger, DBG_DHCP4_BASIC, DHCP4_CLASS_ASSIGNED)
             .arg(classes);
     }
 }}}


 2. If the block is useful, then I see the following:

  2.1 Add todo commentary as to why the block should remain.

  2.2  In the case of the two named append the class name plus a space to
 the string classes, but in the case where you simply add the value from
 the packet option there will be no trailing space.  Shouldn't you do this?

 {{{
         classes += vendor_class->getValue() + " ";
 }}}

  2.3 line 1815 spacing and bracket placement are wrong ;)

 {{{
     }else
     {
 }}}

 should be:

 {{{
     } else {
 }}}

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

 src/bin/dhcp4/dhcp4_srv.cc:

 Dhcpv4Srv::classSpecificProcessing()

 This method is called before the pkt_send call out right?
 User_chk could overwrite the boot file option as it stands the two would
 not
 necessarily be compatible.  Really this is food for thought more than
 anything.

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

 src/lib/dhcp/tests/libdhcp++_unittest.cc

 1. line 1142, this should be an ASSERT_NO_THROW.  If this throws the
 remainder of test is moot.

 2. If all V6 vendor options are constructed of 3 fields: vendor id, data
 len,
 and string at offsets 0, 1, and 2 respectively; shouldn't we at least
 create
 constants for these index values?  This test uses hard-coded numbers for
 indexes and in Dhcp6Srv::classifyPacket() there are several calls to
 vclass->readString(2).  All nice examples of "magic numbers".

 One could even make a case for a derivation of !OptionCustom called
 !VendorCustomOption which provided accessors for id, datalen, and data.


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

 Regarding !ChangeLog entry, I would recommend changing this:

 {{{
 Incoming packets are now assigned to a client class based on
 the content of user class (DHCPv4) or vendor class (DHCPv6).
 }}}

 to this:

 {{{
 Incoming packets are now assigned to a client class based on
 the content of the packet's user class option (DHCPv4) or vendor
 class option (DHCPv6).
 }}}

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