BIND 10 #2320: Allocation engine v4: Address assignment

[BIND 10 Development]

#2320: Allocation engine v4: Address assignment
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  task          |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp4         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130122
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:6 stephen]:
 > Reviewed commit c1fd5c76dc679f0156748f66f58f3d7220703332
 >
 > '''src/bin/dhcp4/dhcp4_messages.mes'''[[BR]]
 > I've done a bit of word-smithing with some of the text in the file and
 pushed the result.
 Thanks.
 > DHCP4_LEASE_ADVERT_FAIL/DHCP4_LEASE_ALLOC_FAIL: I removed the "Each
 specific failure is logged in a separate log entry" text.  I interpreted
 it to mean that each occurrence of this condition would cause the message
 to be logged (in which case the text is not really necessary).  An
 alternative interpretation is that additional messages will indicate the
 reason.  If the latter is the case, please correct the text.
 There may be several reasons why allocation failed. Each specific reason
 will be logged in a separate message. I have updated the text to reflect
 that.

 > '''src/bin/dhcp4/dhcp4_srv.h'''[[BR]]
 > Line 51: Comment: just the presence of the !RequirementLevel enum
 indicates that there is a lot of commonality between the V4 and V6
 servers.  After delivery, we will have a refactoring task to extract the
 common bits into the dhcpsrv library.
 Just because they bear the same name does not mean they work the same.
 Some things can be made common, like the enum you mentioned. But we should
 avoid over-doing things here. In particular sanityCheck() methods only
 look similar, but they work differently. In particular, they use different
 option codes. Client-id is optional in v4 and mandatory in v6. V6 server
 should silently discard received messages that have mismatching server-id
 (which contains a DUID).

 Refactoring just for the sake of saying that something is common while
 there will be two separate functions cramped into one is  not the best
 course of action.

 > '''src/bin/dhcp4/dhcp4_srv.cc'''[[BR]]
 > Lines 270/271:
 > {{{
 > setMsgType(answer, DHCPNAK);
 > answer->setYiaddr(...);
 > }}}
 > Two separate ways of fields in a packet.  It seems more logical that
 setMsgType() should be a method on the Pkt4 class.
 It is not that simple. setYiaddr() sets a field in a fixed part of the
 DHCPv4. It is a trivial setter. setMsgType() is simple, but non-trivial.
 Message type is stored in DHCP Message Type Option.  This function
 searches if there is such an option already (there always should be) and
 updates its value. If there is no such option, it adds a new option.
 Please also note that there is a field in DHCP header that is called op
 and it stores message type, but that is BOOTP message type.

 But you have a point about setting them in the same way. I've moved it to
 Pkt4 and also fixed all tests that were affected by the change (there were
 quite a few - many tests expected that options will have specific pattern
 when packed).

 > Line 273: No need for the "else" clause - the debug message can go in
 the main processing path.
 Done.

 > Line 296-299.  This can be shortened to
 > {{{
 > bool fake_allocation = (question->getType() == DHCPDISCOVER);
 > }}}
 > (Parentheses added to clarify the expression.)
 Done.

 > Line 309: I thought that IA_NA was a DHCPv6 construct. (Looks like a cut
 and paste error in the comment.)
 Cut and paste error indeed. Fixed.

 > setMsgType(): Question: is the logic such that the code may attempt to
 set the message type in a packet in which a type is already set?  If so,
 would an attempt to set a different type indicate a logic error?
 No. When we reply to a REQUEST, we assume that the result with be ACK, but
 we may later discover that the result should be NAK. That's one of the
 many, many reasons I dislike DHCPv4. Packet type doubles here as status
 code.

 > Line 247: getNetmaskOption(): Need documentation of the "subnet"
 parameter.
 Added.

 > Line 319: Question - is another ticket needed to handle the addition of
 any options defined in the configuration database?
 All currently hardcoded options (see HARDCODED_* constants at the top of
 dhcp4_srv.cc) must be implemented. Except that I do not know. Ask Marcin.

 > processRelease(): some comments in the code would be useful: in
 particular, what is the significance of the "if" tests?
 Added.

 > Line 439: Strictly speaking, the return of "false" from the call to
 deleteLease() just means that the lease was not in the database: it does
 not mean that the deletion operation failed.  Such a condition merits a
 warning message, not a debug message.
 It is currently an ERROR message as it indicates database inconsistency,
 some race conditions once we have multi-process capability or someone
 tampering with the database while we are processing this release.

 > Line 439: Continuing from the last comment, the call to deleteLease()
 should be enclosed in a try...catch block.  An exception is generated if
 the deletion fails.  In fact, given that the getLease4() call at line 406
 could also generate an exception, unless exceptions are being caught at a
 higher level, the entire content of this method should be in a try...catch
 block.
 Done.

 > '''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''[[BR]]
 > Please start all comments with a capital letter.
 Done.

 > Even test classes and methods should be Doxygen-documented.
 Added doxygen comments.

 > Line 195: Would prefer that something other than "tmp" is used as a
 variable name (e.g. idopt (for ID option)).  "tmp" should also be changed
 at line 206.
 All tmp renamed to something more meaningful.

 > Line 474 (and others): the "ASSERT_NO_THROW" should not have spaces
 after the leasing "(" and before the trailing ")".
 Removed in several instances.

 > Line 481 (and other lines where the same comment appears): Passing a
 DISCOVER to the server should result in an offer, not an advertise.
 Fixed.

 > Line 920: Comment is incomplete.
 Removed the code and its incomplete comment.

 > Line 872 (and elsewhere): What does the comment "Generate client-id also
 a duid" mean? I thought a DUID was a V6 idea.
 Yet another cut and paste error. Updated comment.

 > Line 993: s/compination/combination/
 Done.

 Rest of the comments will continue in the next update.

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