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:  tmark
                Type:  enhancement   |                       Status:
            Priority:  high          |  reviewing
           Component:  dhcp-ddns     |                    Milestone:  DHCP-
            Keywords:                |  Kea0.9-alpha
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  24            |              Defect Severity:  N/A
         Total Hours:  35            |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  5
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tmark


Comment:

 Replying to [comment:9 tmark]:
 > This was some complicated work, nice job.
 >
 > cppcheck.sh was clean, I did NOT run the unittests through valgrind.

 Thank you QA team. Isn't it your responsibility to run valgrind as well?
 ;) Just kidding.

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

 It is not how the code is working now. The processClientFqdn will not
 append Client FQDN option  if there is no such an option in client's
 message. Client informs of its capability to process this option by
 sending it to the server (and adding its code in ORO). So, you can't
 expect that the FQDN option is always there.

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

 I initially thought that checking for empty hostname is faster than
 forcing writeFqdn to thrown an exception. However, since this is a
 prgrammatic error it should be rare and thus it doesn't really matter.

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

 Added.

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

 What was I thinking?! Fixed.

 >
 >
 ------------------------------------------------------------------------------
 > lib/dhcpsrv/alloc_engine.h
 >
 > There is no commentary for  Lease6Collection::updateFqdnData.

 Yep. I am pretty sure I added this comment and I have no idea what
 happened to it. Added again.

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

 Yes, this test was copied from the V4 test. Fixed.

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

 My editor (everybody knows what it is), does odd things in terms of
 aligning parameters in multiple lines, when doing this change. But I
 forced it to accept it. :)

 >
 >
 ------------------------------------------------------------------------------

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