BIND 10 #1186: libdhcp implementation - DHCPv6 part

[BIND 10 Development]

#1186: libdhcp implementation - DHCPv6 part
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  stephen
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:  Sprint-
                   Priority:  major  |  DHCP-20111026
                  Component:  dhcp   |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DHCP
Feature Depending on Ticket:  878    |  Estimated Difficulty:  0
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by stephen):

 Replying to [comment:9 tomek]:
 '''src/lib/dhcp/option.h'''[[BR]]
 >> addOption(): shared pointer should be passed by reference (to avoid a
 needless copy when passed by value).
 > Certainly not. That is where copy or shared pointer is needed. One
 option (e.g. server-id) may be added in many places, yet due to shared
 pointer only one instance will be created.
 I think you misunderstood the comment.  The idea is that the shared_ptr
 object is passed by reference, not the pointed-to object.  This avoids a
 copy being made of the pointer object when it is passed into the method.

 As it is, the pointer object is copied when it is passed to the method,
 the copy is itself copied when the pair() argument to insert() is
 constructed.  A quick check shows that in the call to insert, a further
 two copies are made of the pointer object, one of which is destroyed when
 the insert() call returns.  And when the call to addOption() returns, the
 copy created for the pair() and the copy created by the call-by-value are
 destroyed.

 In all these copies, there is only ever one instance of the underlying
 object. Passing the argument by reference eliminates one of the pointer
 object copies (and destructor calls).

 This comment applies to a number of methods where a shared_ptr is passed
 by value.

 '''src/lib/dhcp/option.cc'''[[BR]]
 >> pack6()/pack6(): use of an OutputBuffer will automatically take care of
 extending the buffer as needed.
 > Calling realloc() many times is less optimal than calculating length and
 then doing a single new (Pkt6 approach).
 !OutputBuffer can be created with an initial length. If that is exceeded,
 then it grows automatically.  The idea is that (a) the buffer is
 encapsulated and provides methods for writing variable length data items
 to it and (b) if code is changed such that the initial buffer allocation
 is incorrect, it doesn't cause a problem.

 '''src/lib/dhcp/option6_addrlst.h'''[[BR]]
 >> In the constructor for parsing the received option (and in both pack()
 and unpack()), "buf" should be passed by reference.
 > No. This buffer should be kept around for until this option is valid.
 Passing shared pointer be reference is valid only for pure consumers. That
 is clearly not the case here.
 As noted above, passing the shared_ptr object by merely eliminates a copy
 of the pointer object, it does not affect the underlying pointed-to data.
 The pointer is not being altered in this method, it is just being used to
 access the pointed-to data.

 One other thing:

 pack(): C++-style casts should be used (in this case it would be a
 reinterpret_cast<uint16_t*>()).  However, even better would be to use the
 writeUint16 function in io_utilities.h which would eliminate the need for
 a cast.

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