BIND 10 #3295: Refactor DHCPv6 code which processes Client FDQN option.

[BIND 10 Development]

#3295: Refactor DHCPv6 code which processes Client FDQN option.
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  high          |                       Status:
           Component:  dhcp-ddns     |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea0.9-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  24            |                 CVSS Scoring:
         Total Hours:  35            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  5
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * hours:  10 => 5
 * owner:  UnAssigned => marcin
 * totalhours:  30 => 35


Comment:

 This was some complicated work, nice job.

 cppcheck.sh was clean, I did NOT run the unittests through valgrind.

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

 bin/dhcp6/dhcp6_src.cc

 Dhcp6Srv::createNameChangeRequests()

 At line 1056:

 {{{
     // It is likely that client haven't included the FQDN option. In such
 case,
     // FQDN option will be NULL. This is valid state, so we simply return.
 }}}

 If DDNS is enabled we are always going to include the FQDN option in the
 answer so it should be there.  Suppose DDNS is enabled but a programming
 error
 results in there being no FQDN option?  This test would mask such a
 condition.

 I would suggest only calling this method if DDNS is enabled and inside the
 method
 throw an exception if there is no FQDN option.

 ------------------------------------------------------------------------------
 bin/dhcp6/dhcp6_src.cc

 Dhcp6Srv::createRemovalNameChangeRequests()

 Rather than do an explicit check and log message for an empty hostname at
 line 1138, couldn't you just let it process the conversion attempt at
 1153?
 writeFqdn should throw if passed an empty string and both tests guard
 against
 a host name corrupted by outside intervention in the db.

 Also, the log message at 1155 should probably include the lease address.

 ------------------------------------------------------------------------------
 bin/dhcp6/dhcp6_srv.cc

 Dhcp6Srv::generateFqdn()

 At line 2449 you call updateLease6 even if the lease is not found.  In
 other
 words, it is not protected by the lease not null test at 2446.  Really, if
 the lease isn't found shouldn't you throw an exception?  It means
 something
 is rather wrong.

 ------------------------------------------------------------------------------
 lib/dhcpsrv/alloc_engine.h

 There is no commentary for  Lease6Collection::updateFqdnData.

 ------------------------------------------------------------------------------
 lib/dhcpsrv/tests/lease_unittest.c

 TEST(Lease6, hasIdenticalFqdn)

 Curiously this test uses Lease4 instances to compare against.  Not quite
 what you had in mind is it?

 ------------------------------------------------------------------------------
 bin/dhcp6/tests/fqdn_unittest.cc

 Cosmetic but...at line 305 you use the conditional (ternary) operator
 which is great, I love them for this type of thing, but I prefer to
 enclose them in parens like so:


 {{{
         Option6ClientFqdn::DomainNameType fqdn_type = (hostname.empty() ?
             Option6ClientFqdn::PARTIAL : Option6ClientFqdn::FULL);
 }}}

 This way the reader's mind knows the start an expression. It's up to you.

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

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