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:  tmark
                Type:  enhancement   |                       Status:
            Priority:  high          |  reviewing
           Component:  dhcp          |                    Milestone:  DHCP-
            Keywords:                |  Kea0.9-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 tomek):

 * owner:  tomek => tmark


Comment:

 Replying to [comment:9 tmark]:
 > General:
 >
 > Changes passed cppcheck and  unit tests passed valgrind test.
 >
 > 1. You are using hard-coded strings for class names. These should be
 defined as constants somewhere.
 Added constants in docsis3_option_defs.h and libdhcp++.cc.

 > 2. Pkt4::classes_, inClass(), and addClass() are all identical to Pkt6.
 They
 > should be moved to the base class.
 That's an excellent suggestion. The only slight problem is that they do
 not have
 a base class. Added ticket #3296.

 >
 ------------------------------------------------------------------------------
 > 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)
 Added extra text. Let me know if it is satisfactory now.

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


 >
 ------------------------------------------------------------------------------
 > 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:
 Added explanation. In essence, this is simple now, but John B. talked
 about fancier
 stuff there (this is a modem if and only if relay's subscriber-id option
 matches
 client's MAC address). Also, we have one packet capture where the class
 name is
 followed by a space.

 Added explanation for that.
 >
 > 2. If the block is useful, then I see the following:
 >
 >  2.1 Add todo commentary as to why the block should remain.
 Added.

 >  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?
 This is for logging purposes only, so I don't really care if there's extra
 trailing space.

 >  2.3 line 1815 spacing and bracket placement are wrong ;)
 Fixed.

 >
 ----------------------------------------------------------------------------
 >
 > 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.
 Yes. In fact, I'm considering whether we should make a separate hook point
 for this
 (or two - one for doing classification and another one for acting based on
 the classes)
 or not.

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

 > 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".
 It's all magic. Ok, for less magically inclined I added constants in the
 option_vendor
 class. There's no good place to add them (perhaps in std_option_defs.h,
 but that would
 require including that header everywhere), but I think option_vendor class
 is somewhat
 acceptable.

 > One could even make a case for a derivation of !OptionCustom called
 > !VendorCustomOption which provided accessors for id, datalen, and data.
 Not really. The whole idea for OptionCustom was to take care of all such
 odd
 options.

 >
 ----------------------------------------------------------------------------
 >
 > 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).
 > }}}
 Agree. I will use the changelog as suggested.

 Thanks for the review.

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