Opened 8 years ago

Closed 8 years ago

#1186 closed enhancement (fixed)

libdhcp implementation - DHCPv6 part

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20111026
Component: dhcp Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket: 878
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

We need initial implementation for libdhcp. This should cover:

  • DHCPv6 packet definitions
  • DHCPv6 packet parsing/building
  • Generic framework for DHCPv6 options (parsing/storing/processing/modifying/building)

Note: this feature depends on ticket #878.

Subtickets

Change History (27)

comment:1 Changed 8 years ago by tomek

  • Owner changed from tomek to UnAssigned
  • Status changed from new to reviewing

libdhcp implemented. Added support for IPv6 (There's separate ticket for IPv4). Added classes for option handling, together with many tests.

Ready for review.

Note for reviewers:

  • This depends on 878 (currently under review). It was branched off from trac878.
  • IPv4 support is not present, there's separate ticket for that
  • log4cplus support it still to be added

comment:2 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:3 follow-up: Changed 8 years ago by stephen

Review of commit f56892b514cedb0538c37db808ec543d7a4fcfaf - part 1

As a large number of files have been altered, the review is in two parts. The first part concerns the files in src/bin/dhcp6 (and associated tests).

Notes:

  • It has been noted that there is a TODO to convert the files to use BIND 10 logging. Therefore no comments have been made about this.

src/bin/dhcp6/dhcp6_srv.cc
The #include of 'io_address.h' should be #include "asiolink/io_address.h"

In Dhcpv6Srv::run(), there is the following construct:

if (rsp != boost::shared_ptr<Pkt6>()) {

To check whether rsp holds a valid pointer, the simpler if (rsp) { can be used.

getServerID(): Presumably there is a getServerID() as a counterpart to setServerID()? (If not, why not access serverid_ directly?) If so, it would make sense to have getServerID() return a reference to serverid_ to save on the copying of the shared pointer. Also, getServerID() is sufficiently trivial that it can be coded inline in the header.

In setServerID(), need to check that srvid[3] should really be 6.

Does setServerID() need to return anything? If the code fails to set the server ID, would an exception be a better way of signalling it?

in processSolicit(), the code contains two declarations of the form Option * tmp. This should be Option* tmp (as the current layout looks at first glance like a multiplication). Also, should be no space between the ">" and "(" in the dynamic_cast statement.

Should include a 'TODO:' comment about changing them in the code for the hard-wired times and address.

src/bin/dhcp6/dhcp6_srv.h
To avoid a copy of the input variable, the solicit argument to many of the methods should be passed by reference.

All methods in Dhcpv6Srv should have a Doxygen header.

The class Dhcp6Srv should have a header explaining the purpose of the class.

The convention in BIND 10 is to use nested namespaces rooted at "isc" for different components. Suggest that the class be put into isc::dhcp.

Declaring the test class as a "friend" in the production code is unusual (in BIND 10) but valid. This should be discussed on bind10-dev as to whether we want to make this a standard. However, if the test class is a friend, why are methods protected?

To prevent copying, the class should be derived from boost::noncopyable. (In fairness, although this was decided back in February, is has only today been included into the coding guidelines.)

All member variables should include a Doxygen comment stating their purpose.

src/bin/dhcp6/iface_mgr.cc
(A few things that may have been picked up in the review of #868 but caught my eye when reviewing these changes.)

instance(): the if statement should contain braces, even if the contents are only one statement.

Iface constructor. For setting the mac_ array to zero, memset is OK (although some authorities prefer std::fill). However the length should be sizeof(mac_) and not a hard-coded 20.

getFillName(): return (name_ + "/"); is simpler than creating a temporary ostringstream object.

getPlainMac(): I think that setting the fill, width and radix once outside the for loop should be OK. (Also, if mac_ is declared as uint8_t, there should be no need for the cast to an int when output each element of it.)

In IfaceMgr constructor, control_buf_ could be declared as a boost::scoped_ptr<> to provide automatic destruction in the case that anything else in the constructor throws an exception. (Would then remove the deallocation of control_buf_ from the destructor.)

Header for openSocket. The BIND 10 convention is to place the headers in the .h file. Also, the first line of the header contains a typo "nad" instead of "and".

receive(): without checking where this is called from, would not throwing an exception (instead of returning an empty packet) be a better way of signalling that the receive has failed?

src/bin/dhcp6/iface_mgr.h
See comment in dhcp6_srv.h about namespaces.

All methods - including those in the contained Iface struct - should have Doxygen headers.

Remove the note that struct Iface could be a class as well - in this context, a struct is fine.

mac_ is best declared as uint8_t. (It is not being used as a char.)

Should remove the final comment in struct Iface as there is no next field.

Suggest grouping the variables and the method declarations in struct Iface instead of putting the latter in the middle of the former.

Iface * getIface(...) should be Iface* getIface(...) (looks like a multiplication). This appliease to other similar declarations in the file.

send() should take a reference to a shared pointer as its argument. Also, if the argument is unchanged, it should be const.

All member variables should include a Doxygen comment stating their purpose.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
Basic test. The comment says that "an attempt to bind this socket will fail" yet the test is EXPECT_NO_THROW - which indicates that no exception will be thrown when the test succeeds.

Should be a space after the comma in the TEST_F line for Solicit_basic.

Inconsistency in declaration: both sol and ia are of the form boost::shared_ptr<xyz>, yet the former is initialized using the construct "ptr x = ptr(new Xyz(...))" and the latter "ptr x(new Xyz(...))".

Would help to include a comment explaining what options are being set in the packet.

See above for comments on the testing whether a shared pointer points to a null object.

In the Solicit_basic test, the contents of the ASSERT_TRUE macros can simply be the shared_ptr in question - the implicit conversion to bool will return true if the pointed is non-null.

src/bin/dhcp6/tests/iface_mgr_unittest.cc
In the test loDetect, can you guarantee that the default directory when running the test is writeable and that "interfaces.txt" will be created? In fact, removing "interfaces.txt" and instead using an environment variable might be safer. (Which will require an update to the IfaceMgr code.)

In the "sockets" test, the socket numbers are perhaps best put as constants at the head of the file (in case they need changing at some time in the future).

In test sendReceive, the ASSERT_TRUE condition can be just rcvPkt (see above).

comment:4 Changed 8 years ago by stephen

  • Milestone changed from DHCP 2011 to Sprint-DHCP-20111011

comment:5 follow-ups: Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

Review of commit f56892b514cedb0538c37db808ec543d7a4fcfaf - part 2

src/lib/asiolink/io_address.h
The "data" argument in "from_bytes" should be declared "const uint8_t*" as it is binary data.

There is the possibility that in use, a data buffer of the wrong length will be passed through. Is it feasible/useful to also pass through a buffer length, and have the method check that as well?

src/lib/asiolink/io_address.cc
IOAddress::from_bytes(): the buffer addr_str does not need to be static. Also, it can be declared closer to the point of first use, immediately before the call to inet_ntop().

IOAddress::from_bytes(): addr_str is declared as length INET6_ADDRSTRLEN. In fact, as it will hold either an IPv4 address string or an IPv6 address string, the length should really be max(INET6_ADDRSTRLEN, INET_ADDRSTRLEN). True, the IPv6 constant is larger, but I would add a:

BOOST_STATIC_ASSERT(INET6_ADDRSTRLEN >= INET_ADDRSTRLEN)

...to the code to emphasise the dual nature of the buffer.

IOAddress::from_bytes(): missing space before INET6_ADDRSTRLEN in call to inet_ntop().

IOAddress::from_bytes(): minor semantic point - I suggest that the second "if" be an "else if", to emphasise that this is part of the "if" block checking for the validity of the arguments.

src/lib/dhcp/Makefile.am
Why is "-Wall" added to the C++ flags varianble. Also, the comment about turning off -Werror does not appear to be relevant here.

You should not need to include any LOG4CPLUS symbols in the lipdhcp_la_xxx symbol definitions; those should be hidden by the logging library.

src/lib/dhcp/dhcp6.h
Seems to be a mis-alignment of the values for options codes 6, 13, 17, 27, 39, 43 and 45; the values should be vertically aligned (looks neater).

#define is generally discouraged in C++, with "const" declarations being preferred. In this case, as there is a relationship between the D6O values - they are DHCPv6 options - using an "enum" would seem to be better. Each values can be implicitly converted to an "int", although the main advantage is that some form of type checking can be employed where the value is passed as an argument.

Putting the declarations into a enum also allows the omission of the prefixes (e.g.

#define STATUS_Success 0

... becomes ...

enum DHCPv6Status {
   Success = 0,
        :
};

(... although I would suggest retaining a prefix for DHCPv6 options but changing it from D6O_ to OPTION_, so as to match up with the entries in the IANA registry.)

Suggest the declarations of DHCPv4 and DHCPv6 symbols are put into separate namespaces. In this way the prefix to the symbol names can be omitted, with the namespace being used to disambiguate options with similar names.

Suggest changing the D6O prefix to OPTION_ to indicate a V6 option (this is used in the IANA registry). Also, perhaps STATUS6_ and MSG6_ to indicate V6 status codes and message types?

Should perhaps indicate in which RFC options 2 through 20 are defined. Also, what is a "paa-option" (option 40)?

src/lib/dhcp/libdhcp.h
The idea of a "LibDHCP" class is not really object oriented - it seems to imply a general collection of loosely-related functions, which is more of a C paradigm. What we have in this class seems more to be an OptionCollection object - an object capable of disassembling and assembling

Related to the above, there is are packOptions6() and unpackOptions6() methods and an OptionFactoryRegister() method that requires an indication of the "universe" in which it operates. All of these seem to be arguing for two classes, one for v6 and one for v4, each with pack, unpack and factoryRegister methods.

No version() method is required - versioning of all BIND 10 libraries will be taken care of in a general manner.

src/lib/dhcp/libdhcp.cc
unpackOptions6(): Using the InputBuffer class in "util" allows items of data to be read from a buffer in 16-bit words.

unpackOptions6(): Should be spaces around relational operators (in "while (offset<end)").

unpackOptions6(): The "while" loop just checks for offset < end. The extraction of the type and length fields require that four bytes be present. The check should be "offset + 4 <= end" (to allow for an option with no data).

unpackOptions6(): Presumably unpack6() will ultimately use the factories registered in this class?

packOptions6(): If "options" is not being changed, can the argument be "const" and the iterator a const_iterator?

packOptions6(): Using an OutputBuffer from the util directory (instead of the shared array of char) will allow for automatic extension of the buffer as the options are added.

src/lib/dhcp/option.h
Is there a case for separate Option6 and Option4 classes, possibly derived from an abstract base Option class?

Would suggest that the typedefs OptionXCollection (instead of OptionXLst) be used, as a "Lst" suffix implies a list.

All method declarations - including constructors - should have full Doxygen headers.

unpack(): given that the option data in the buffer includes a length field that is the length of the option data, does unpack() really need the parse_len option?

unpack(): Offsets into the buffer (and buffer sizes) should be size_t instead of "unsigned int".

toText(): no other toText() method in BIND 10 takes an indentation argument.

addOption(): shared pointer should be passed by reference (to avoid a needless copy when passed by value).

getOption()/delOption() should be renamed getSubOption()/delSubOption().

Related to suboptions: would it be better for the Option class to supply an iterator (and associated begin()/end()) methods to allow iterations over suboptions contained within an option? That way, all STL algorithms could be used on the suboption collection.

pack4()/pack6() - see suggestion about two Option classes, one for V4 and one for V6.

What is optionLst_?

Is there any reason why the data members are protected instead of private?

src/lib/dhcp/option.cc
pack6()/pack6(): use of an OutputBuffer will automatically take care of extending the buffer as needed.

pack4(): not clear why data_len_+4 bytes are being copied into the output buffer. The type and length have already been copied and the pointer into the buffer advanced. According to the comment in the header file, data_len_ is the length of data only, so copying data_len_+4 risks copying beyond the allocated data area. (Note that only data_len_ bytes are copied in pack6().)

len(): need comment as to why the length is increments by the length of the contents of optionLst_. I am assuming that these are suboptions, but there is nothing here or in the header file to indicate that.

valid(): the comments here do not relate to the code.

valid(): A simpler statement is return ((universe == V4) || (universe == V6));.

valid(): Under what circumstances (other than wrong universe) can an option be invalid?

addOption(): argument should be passed by reference.

getOption()/delOption(): should be spaces around the "!=".

delOption(): the intended result of the method is that after execution, the specified option is not in the collection. If it wasn't there to begin with, after the method call the result is the same, the option is not there. In this case, is it justified to return false? A second question is, if this is an unexpected occurrence, isn't throwing an exception the better way of signalling it?

src/lib/dhcp/option6_addrlst.h
In the typedef, suggest that AddressContainer is easier to read than AddrsContainer. (The latter only saves two characters.)

In the constructors, the address(es) should be passed as const.

In the simplified constructor, the single address should be passed by reference.

In the constructor for parsing the received option (and in both pack() and unpack()), "buf" should be passed by reference.

In setAddress(), the argument should be passed by reference and should be const.

In setAddresses(), the argument should be const.

src/lib/dhcp/option6_addrlst.cc
Constructor taking a vector: addrs_ can be initialised in the member variable initializer list with addrs_(addrs).

Constructor taking a single IOAddress: addrs_ can be initialized in the member variable initialiser list with addrs_(1, addrs).

pack()/unpack(): use of "magic number" of 16. Better is to use a named constant.

toText(): would suggest that this method just returns the addresses as a simple space-separated list. This will make it easier to use in log messages and other places where a list is required.

len(): suggest that the DHCPv6 option header length is defined as a constant somewhere.

src/lib/dhcp/option6_ia.h
Not sure why Option6IA constructor needs a "Universe" parameter. Doesn't this option only exist in V6, in which case there is no need for the Option6IA constructor to have it as an argument; when calling the base constructor, it could pass V6 across explicitly.

Although overridden methods do not need a full Doxygen header, methods declared in this class - even trivial ones - should have such documentation.

Arguments in argument lists should be on the same line where possible (up to a line length of 80).

Constructor/pack()/unpack(): buf should be passed by reference to avoid a needless copy of the shared_array object.

src/lib/dhcp/option6_ia.cc
pack(): should use OutputBuffer to take account of field extension.

pack(): there should be a comment (and possibly an assert()) to ensure that len() returns a value >= 16.

pack(): when incrementing the pointer, something like "ptr += sizeof(uint32_t)" is more descriptive that "ptr += 4" when coming immediately after the insertion of a uint32_t.

unpack(): should be spaces around the relational and arithmetic operators.

toText(): would suggest that this method just returns the addresses as a simple space-separated list. This will make it easier to use in log messages and other places where a list is required. If there is a need to identify the type of this option, then there should be a method that returns a class describing the option type, this class having its own toText() method.

src/lib/dhcp/option6_iaaddr.h
Although overridden methods do not need a full Doxygen header, methods declared in this header - even trivial ones - should have such documentation.

Arguments in argument lists should be on the same line where possible (up to a line length of 80).

Constructor/pack()/unpack(): buf should be passed by reference to avoid a needless copy of the shared_array object.

setAddress(): argument should be const and passed by reference.

src/lib/dhcp/option6_iaaddr.cc
Second constructor: when initializing addr_, just pass the string "::" as the argument. As it is, an anonymous IOAddress object is being created then addr_ is being initialized via the copy constructor.

pack(): reference to the magic "16". This should be a symbolic constant.

pack(): same comment as above concerning the incrementing of the ptr object.

Note: the part of pack() that adds the type and length to the header is common to both Option6IA and Option6IAAddr; presumably it applies to all V6 options. This suggests that it should be a method in the base class that can be called by all derived classes. (It would also mean that the type_ field can remain private to the base class.)

Similarly, the only difference between Option6IA::len() and Option6IAAddr::len() is the size of the header; the rest of the code in the method is concerned with summing the length of the suboptions. This suggests that the code for summing suboption lengths could be put in the base Option class. Better though would be to define Option6Lst as a separate class and provide a len() method within it (as it is returning an attribute of the object).

src/lib/dhcp/pkt6.h
Public enums should not end with an underscore. (Trailing underscores are used in private member variables.)

Its is not BIND 10 convention to indent the class definition within a namespace.

All methods must have Doxygen headers.

Inconsistent definition of default values in the Pkt6 constructor and setProto (UDP v Pkt6::UDP)

TODO comment says that member fields should be protected - they should be private.

Member variables should be commented using Doxygen constructs.

addOption(): argument should be passed by reference.

Ideally inline comments for member variables should line up.

It's confusing to mix up member variable and member function declarations.

src/lib/dhcp/pkt6.cc
The two constructors could be combined into one if default arguments were used.

All member variables that are basic types (ints etc.) should be initialized in the constructor, else their value is not defined.

len(): uses the same code as used in the Option class. See above for comments.

packUDP(): if the system fails to allocate memory, this is fairly fatal. It seems logical to allow the exception to propagate instead of returning a value as there is little than can be done.

unpackUDP(): data_ is declared as a shared_array of char, yet the unpack method casts individual bytes to unsigned char. Suggest declaring data_ as a shared_array of uint8_t.

getType(): this is sufficiently trivial that it could be defined in the header file.

As there are no virtual functions in the class, there is no need to declare and define a destructor - the default one provided by the compiler will be sufficient.

src/lib/dhcp/tests/libdhcp_unittest.cc
Spaces required around relational and arithmetic operators and before/after equal signs.

packOptions6 test: "expected" array is missing comment for opt5.

unpackOptions6 test: "packed" array is the same as the "expected" array in the previous test. The declaration could be shared between them.

src/lib/dhcp/tests/option6_addrlst_unittest.cc
basic test: Comment - sampledata, expected1, expected2 and expected3 contain much the same data. Isn't there some way to declare it once and create the arrays dynamically (so avoiding the chance of errors if the code is changed in the future). The same goes for the text representation of the addresses.

Is there a reason to use IPV6 addresses other than 2001:db8::/32 for tests (as required in the coding guidelines). If so, the reason should be documented.

All tests. Is there a need to declare opt1/2/3 as a pointer to Option6AddrLst and delete them afterwards? As they are being initialized at creation, why not declare them as objects of type Option6AddrLst? It eliminates the need to delete them at the end of the test method. (Alternatively, encapsulate them in a std::auto_ptr to ensure that they are deleted even if the method returns prematurely.)

src/lib/dhcp/tests/option6_ia_unittest.cc
Spaces required around relational and arithmetic operators and before/after equal signs.

Do the Option6IA objects need to be created via "new"? Can't they be automatically allocated (and so avoid the need for "delete" statements?)

src/lib/dhcp/tests/option6_iaaddr_unittest.cc
Spaces required around relational and arithmetic operators and before/after equal signs.

basic test: we are using US conventions in documentation, so the comment by simple_buf[23] should read "3,000,000,000" (i.e. commas, not periods)

There should be a test for len().

src/lib/dhcp/tests/option_unittest.cc
Do the Option objects need to be created via "new"? Can't they be automatically allocated (and so avoid the need for "delete" statements?)

src/lib/dhcp/tests/pkt6_unittest.cc
Is there a reason to use IPV6 addresses other than 2001:db8::/32 for tests (as required in the coding guidelines). If so, the reason should be documented.

capture1(): wouldn't it be easier to declare the data in a 98-element array and then memcpy it to pkt->data? (As it is, the columns of the set of assignments do not line up).

parse_solicit1: as this is a test of unpack(), the test might be better named unpack_solicit1.

No tests for len(), packUPD(), unpackUDP(), getType().

comment:6 follow-up: Changed 8 years ago by stephen

Review of commit f56892b514cedb0538c37db808ec543d7a4fcfaf - part 2 (cont)

One other thing...

src/lib/asiolink/tests/io_address_unittest.cc
This fails to compile. The from_bytes test includes the line

EXPECT_EQ(addr, IOAddress("192.0.2.3"));

This fails to compile because IOAddress does not support operator<< (used for error messages). It needs to be changed to

EXPECT_TRUE(addr == IOAddress("192.0.2.3"));

src/lib/dhcp/tests/libdhcp_unittest.cc
Fails to compile for a similar reason. Lines of the form:

ASSERT_NE(x, options.end());
EXPECT_EQ(x, options.end());

Need to be changed to

ASSERT_FALSE(x == options.end());
EXPECT_TRUE(x == options.end());

src/lib/dhcp/tests/option6_iaaddr_unittest.cc
Compilation fails with the message:

option6_iaaddr_unittest.cc:79: error: this decimal constant is unsigned only in ISO C90

(The constant in question is 3000000000, which is larger than the maximum signed positive 32-bit integer.)

src/lib/dhcp/tests/option6_ia_unittest.cc
Compilation fails with the message:

option6_ia_unittest.cc: In member function ‘virtual void<unnamed>::Option6IATest_suboptions_unpack_Test::TestBody()’:
option6_ia_unittest.cc:214: error: ‘ia’ may be used uninitialized in this function

src/lib/dhcp/iface_mgr.cc
Fails to compile:

iface_mgr.cc: In member function ‘int isc::IfaceMgr::openSocket(const std::string&, const isc::asiolink::IOAddress&, int)’:
iface_mgr.cc:256: error: dereferencing pointer ‘addr6’ does break strict-aliasing rules
iface_mgr.cc:254: error: dereferencing pointer ‘addr6’ does break strict-aliasing rules
iface_mgr.cc:255: error: dereferencing pointer ‘addr6’ does break strict-aliasing rules
iface_mgr.cc:253: note: initialized from here

(There may be other compilation errors, but I didn't check further.)

comment:7 in reply to: ↑ 3 ; follow-up: Changed 8 years ago by tomek

Replying to stephen:

src/bin/dhcp6/dhcp6_srv.cc
The #include of 'io_address.h' should be #include "asiolink/io_address.h"

Done. In dhcp6_srv.cc and other files in src/bin/dhcp6 and src/lib/dhcp.

In Dhcpv6Srv::run(), there is the following construct:

if (rsp != boost::shared_ptr<Pkt6>()) {

To check whether rsp holds a valid pointer, the simpler if (rsp) { can be used.

I think that is against BIND9 conding standards that is also used in DHCP. It seems that is ok in BIND10, though, so I changed it to use simpler form.

getServerID(): Presumably there is a getServerID() as a counterpart to setServerID()? (If not, why not access serverid_ directly?) If so, it would make sense to have getServerID() return a reference to serverid_ to save on the copying of the shared pointer. Also, getServerID() is sufficiently trivial that it can be coded inline in the header.

Done.

In setServerID(), need to check that srvid[3] should really be 6.

Certainly not. While 6 is the most common hardware type (Ethernet), we must not limit ourselves to this hardware type. In particular, Infiniband is represented by hardware type 32, which is a valid type. Anyway, there's explicit statement in RFC3315 that says that DUID content MUST be treated as opaque value. If user desires to have it composed with values other than 6, it is legal.

Does setServerID() need to return anything? If the code fails to set the server ID, would an exception be a better way of signalling it?

Changed as suggested. Once fully implemented, setServerID() will throw exception if it fails to load old DUID and there are no suitable interfaces for generation of new DUID.

in processSolicit(), the code contains two declarations of the form Option * tmp. This should be Option* tmp (as the current layout looks at first glance like a multiplication). Also, should be no space between the ">" and "(" in the dynamic_cast statement.

Done.

Should include a 'TODO:' comment about changing them in the code for the hard-wired times and address.

Added TODO explaining that this method should be rewritten once LeaseManager? is implemented.

src/bin/dhcp6/dhcp6_srv.h
To avoid a copy of the input variable, the solicit argument to many of the methods should be passed by reference.

No. In current state of implementation, packet is consumed immediately and is not stored anywhere. That is likely to change, however. Note that leasequery protocol may ask about details about relays, so it may be useful to keep at last some parts of message. Also, if we decide to implement distributed processing, we need separate instance of shared_pointer.

All methods in Dhcpv6Srv should have a Doxygen header.

Done.

The class Dhcp6Srv should have a header explaining the purpose of the class.

Done.

The convention in BIND 10 is to use nested namespaces rooted at "isc" for different components. Suggest that the class be put into isc::dhcp.

Done. Note that library, dhcp6 and upcoming dhcp4 components will all use a single (isc::dhcp) namespace.

Declaring the test class as a "friend" in the production code is unusual (in BIND 10) but valid. This should be discussed on bind10-dev as to whether we want to make this a standard. However, if the test class is a friend, why are methods protected?

Use of a friend was a quick hack, rather than permanent solution. Implemented derived class that exposes Dhcpv6Srv and removed friend statement.

To prevent copying, the class should be derived from boost::noncopyable. (In fairness, although this was decided back in February, is has only today been included into the coding guidelines.)

Done.

All member variables should include a Doxygen comment stating their purpose.

Done.

src/bin/dhcp6/iface_mgr.cc
(A few things that may have been picked up in the review of #868 but caught my eye when reviewing these changes.)

instance(): the if statement should contain braces, even if the contents are only one statement.

Done.

Iface constructor. For setting the mac_ array to zero, memset is OK (although some authorities prefer std::fill). However the length should be sizeof(mac_) and not a hard-coded 20.

Using std::fill is a overkill. modified memset should do the trick. Also added MAX_MAC_LEN const.

getFillName(): return (name_ + "/"); is simpler than creating a temporary ostringstream object.

No, it is not. It is not possible to add integer to strings. using stringstream is the preferred way of converting integers to strings in C++. Note that the format is eth0/2 (interface-name, a slash followed by integer representing interface index).

getPlainMac(): I think that setting the fill, width and radix once outside the for loop should be OK. (Also, if mac_ is declared as uint8_t, there should be no need for the cast to an int when output each element of it.)

Note: As some manipulator affect only the next token (and I fail to remember which ones they are) I chose to set the before every token.

To answer this comment: Yes and no. uint8_t is still printed as char, so cast to int is required, at least with g++ 4.5.2. Some stream manipulators apply to next token only. In particular that is true for width(). This page has nice table about applicability of stream manipulators: http://www.fredosaurus.com/notes-cpp/io/omanipulators.html

Cast to (int) stays, fill() moved out of loop.

In IfaceMgr constructor, control_buf_ could be declared as a boost::scoped_ptr<> to provide automatic destruction in the case that anything else in the constructor throws an exception. (Would then remove the deallocation of control_buf_ from the destructor.)

scoped_ptr is not the proper type here as it will call delete to destroy pointer object. This is an array, so delete [] operator should be called. Note that deleting array type with delete results in undefined behavior. It just happens to work in g++.

The better template would be shared_array<>. Do you want me to use it?

Header for openSocket. The BIND 10 convention is to place the headers in the .h file. Also, the first line of the header contains a typo "nad" instead of "and".

receive(): without checking where this is called from, would not throwing an exception (instead of returning an empty packet) be a better way of signalling that the receive has failed?

No. Once LeaseManager? is implemented, this method will get a timeout timer ("try to receive something for 10 seconds. After that we need to expire a lease"). Not receiving anything will be a proper event.

src/bin/dhcp6/iface_mgr.h
See comment in dhcp6_srv.h about namespaces.

Done.

All methods - including those in the contained Iface struct - should have Doxygen headers.

Done.

Remove the note that struct Iface could be a class as well - in this context, a struct is fine.

Done.

mac_ is best declared as uint8_t. (It is not being used as a char.)

Done.

Should remove the final comment in struct Iface as there is no next field.

This was an explanation why there is no such field. Removed as suggested.

Suggest grouping the variables and the method declarations in struct Iface instead of putting the latter in the middle of the former.

Done.

Iface * getIface(...) should be Iface* getIface(...) (looks like a multiplication). This appliease to other similar declarations in the file.

Done.

send() should take a reference to a shared pointer as its argument. Also, if the argument is unchanged, it should be const.

I prefer to keep it as separate copy of shared pointer. We may decide to use async send sometime later.

All member variables should include a Doxygen comment stating their purpose.

Done.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
Basic test. The comment says that "an attempt to bind this socket will fail" yet the test is EXPECT_NO_THROW - which indicates that no exception will be thrown when the test succeeds.

See IfaceMgr::IfaceMgr?() comment. This exception is caught prematurely. It should stay that way until proper interface detection is implemented. I really don't want to engineer a work-around for a test that tests stub implementation that is going away in 2 months.

Should be a space after the comma in the TEST_F line for Solicit_basic.

Added.

Inconsistency in declaration: both sol and ia are of the form boost::shared_ptr<xyz>, yet the former is initialized using the construct "ptr x = ptr(new Xyz(...))" and the latter "ptr x(new Xyz(...))".

Done.

Would help to include a comment explaining what options are being set in the packet.

Added a comment about content of "sent" message and expected content of "received" message.
Also added several new checks.

See above for comments on the testing whether a shared pointer points to a null object.

In the Solicit_basic test, the contents of the ASSERT_TRUE macros can simply be the shared_ptr in question - the implicit conversion to bool will return true if the pointed is non-null.

Done.

src/bin/dhcp6/tests/iface_mgr_unittest.cc
In the test loDetect, can you guarantee that the default directory when running the test is writeable and that "interfaces.txt" will be created? In fact, removing "interfaces.txt" and instead using an environment variable might be safer. (Which will require an update to the IfaceMgr code.)

I politely refuse to implement this. This is a temporary workaround that is expected to go away in less than 2 months. It is not perfect, I agree. However, at this stage of development, write access to current directory during running tests is required. Added comment that explain that.

Let's not over-engineer a workaround. We risk making it a feature.

In the "sockets" test, the socket numbers are perhaps best put as constants at the head of the file (in case they need changing at some time in the future).

These are just random numbers. Well, almost. DHCPv6 operates on 546 and 547 ports. I used normal value+10000 to have the ability to run as non-root user. Defining constants would be confusing. Tests would use something like DEFAULT_DHCPV6_PORT_PLUS_10000 + 1. It would be more confusing than directly using numbers. They are not expected to be unified among different tests. On the contrary, using different values would increase test coverage.

In test sendReceive, the ASSERT_TRUE condition can be just rcvPkt (see above).

Done.

Commit-id: 8fdcf2aa39b5fdb224f5e57f9605090409abc4a1

comment:8 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by tomek

Replying to stephen:

src/lib/asiolink/io_address.h
The "data" argument in "from_bytes" should be declared "const uint8_t*" as it is binary data.

Done.

There is the possibility that in use, a data buffer of the wrong length will be passed through. Is it feasible/useful to also pass through a buffer length, and have the method check that as well?

It is possible, but not very useful. Eventually we will have to call inet_ntop() and see if the string is sane.

src/lib/asiolink/io_address.cc
IOAddress::from_bytes(): the buffer addr_str does not need to be static. Also, it can be declared closer to the point of first use, immediately before the call to inet_ntop().

Done.

IOAddress::from_bytes(): addr_str is declared as length INET6_ADDRSTRLEN. In fact, as it will hold either an IPv4 address string or an IPv6 address string, the length should really be max(INET6_ADDRSTRLEN, INET_ADDRSTRLEN). True, the IPv6 constant is larger, but I would add a:

BOOST_STATIC_ASSERT(INET6_ADDRSTRLEN >= INET_ADDRSTRLEN)
...to the code to emphasise the dual nature of the buffer.

Done.

IOAddress::from_bytes(): missing space before INET6_ADDRSTRLEN in call to inet_ntop().

IOAddress::from_bytes(): minor semantic point - I suggest that the second "if" be an "else if", to emphasise that this is part of the "if" block checking for the validity of the arguments.

Done as suggested. Nevertheless, the compiled code will be exactly the same in both cases. In my opinion previous code was more readable. That is not a problem in such simple code, so it doesn't matter much.

src/lib/dhcp/Makefile.am
Why is "-Wall" added to the C++ flags varianble. Also, the comment about turning off -Werror does not appear to be relevant here.

You should not need to include any LOG4CPLUS symbols in the lipdhcp_la_xxx symbol definitions; those should be hidden by the logging library.

Done.

src/lib/dhcp/dhcp6.h
Seems to be a mis-alignment of the values for options codes 6, 13, 17, 27, 39, 43 and 45; the values should be vertically aligned (looks neater).

#define is generally discouraged in C++, with "const" declarations being preferred. In this case, as there is a relationship between the D6O values - they are DHCPv6 options - using an "enum" would seem to be better. Each values can be implicitly converted to an "int", although the main advantage is that some form of type checking can be employed where the value is passed as an argument.

That file is a verbatim import from ISC DHCP. Due to possible code sharing, I prefer to not implement any changes, unless necessary. I fixed offset, but I will leave defines are they are for now.

Putting the declarations into a enum also allows the omission of the prefixes (e.g.

#define STATUS_Success 0

... becomes ...

enum DHCPv6Status {
   Success = 0,
        :
};

(... although I would suggest retaining a prefix for DHCPv6 options but changing it from D6O_ to OPTION_, so as to match up with the entries in the IANA registry.)

That would be useful. Having options named exactly the same way as in RFC would be nice. Unfortunately, that means that there will be a conflict in ISC DHCP. That was the reason to start with D6O_. We need to decide if we want to maintain dhcp6.h as a common file or not.

Suggest the declarations of DHCPv4 and DHCPv6 symbols are put into separate namespaces. In this way the prefix to the symbol names can be omitted, with the namespace being used to disambiguate options with similar names.

Will do this during work on DHCPv4 part.

Suggest changing the D6O prefix to OPTION_ to indicate a V6 option (this is used in the IANA registry). Also, perhaps STATUS6_ and MSG6_ to indicate V6 status codes and message types?

Should perhaps indicate in which RFC options 2 through 20 are defined. Also, what is a "paa-option" (option 40)?

I have no idea. This file came from ISC DHCP.

src/lib/dhcp/libdhcp.h
The idea of a "LibDHCP" class is not really object oriented - it seems to imply a general collection of loosely-related functions, which is more of a C paradigm. What we have in this class seems more to be an OptionCollection object - an object capable of disassembling and assembling

Related to the above, there is are packOptions6() and unpackOptions6() methods and an OptionFactoryRegister() method that requires an indication of the "universe" in which it operates. All of these seem to be arguing for two classes, one for v6 and one for v4, each with pack, unpack and factoryRegister methods.

That is a work in progress. The idea is to

No version() method is required - versioning of all BIND 10 libraries will be taken care of in a general manner.

version() method removed.

src/lib/dhcp/libdhcp.cc
unpackOptions6(): Using the InputBuffer class in "util" allows items of data to be read from a buffer in 16-bit words.

InputBuffer? is unsuitable here. Hooks may modify incoming packet, so input buffer is actually read-write.

unpackOptions6(): Should be spaces around relational operators (in "while (offset<end)").

unpackOptions6(): The "while" loop just checks for offset < end. The extraction of the type and length fields require that four bytes be present. The check should be "offset + 4 <= end" (to allow for an option with no data).

unpackOptions6(): Presumably unpack6() will ultimately use the factories registered in this class?

Yes, it will eventually. Option extension framework is not planned for phase 1, however.

packOptions6(): If "options" is not being changed, can the argument be "const" and the iterator a const_iterator?

Added const in packOptions6 (const options) and unpackOptions6 (const buffer).

packOptions6(): Using an OutputBuffer from the util directory (instead of the shared array of char) will allow for automatic extension of the buffer as the options are added.

Changing type of packet depending on if it is received or sent is not a good idea in DHCP. Why changing type of packet container? That would double number of required methods (or introduction of a packet hierarchy).

Also, calling realloc many times is inefficient. In DHCP, size of the buffer is known once all options are added to buffer. There's len() method that returns number of bytes needed. A single realloc() is sufficient.

src/lib/dhcp/option.h
Is there a case for separate Option6 and Option4 classes, possibly derived from an abstract base Option class?

v4 and v6 options are structured completely different. type and length fields are of different sizes, there are 1-byte long options in DHCPv4, string encoding is sometimes different etc. I can hardly find any reasonable case where a method could handle both v4 and v6 options.

Commit-id: e4ad0dcba0cd6ab978b826b824f663596349875c
Rest of the review will continue in the next comment (tomorrow).

comment:9 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by tomek

Replying to stephen:

Review of commit f56892b514cedb0538c37db808ec543d7a4fcfaf - part 2

Would suggest that the typedefs OptionXCollection (instead of OptionXLst) be used, as a "Lst" suffix implies a list.

Renamed.

All method declarations - including constructors - should have full Doxygen headers.

Documented.

unpack(): given that the option data in the buffer includes a length field that is the length of the option data, does unpack() really need the parse_len option?

Yes. Because sometimes only certain size of the buffer needs to be parsed. One notable example is when you parse an option that contains suboptions. You don't want to parse till end of the buffer, but just a specific length. That length is obtained when parsing upper level option, so it cannot be derived here. Therefore needs to be passed as a parameter.

unpack(): Offsets into the buffer (and buffer sizes) should be size_t instead of "unsigned int".

Agree. This also applies to many other method and code pieces throughout the whole libdhcp and dhcp6. Added ticket #1295 for that.

toText(): no other toText() method in BIND 10 takes an indentation argument.

That implies that no other code in BIND10 deals with nested options. The idea here is that for nested options, each option will be printed indented for clearer representation. I understand that it is a new concept, so the default value of indent parameter is 0. The usual usage stays the same.

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.

getOption()/delOption() should be renamed getSubOption()/delSubOption().

I would prefer to keep it as getOption/delOption. This way, regardless of the object (a packet, an option or a cache), the same method can be used (getOption). A shorter names are easier to type.

Related to suboptions: would it be better for the Option class to supply an iterator (and associated begin()/end()) methods to allow iterations over suboptions contained within an option? That way, all STL algorithms could be used on the suboption collection.

That is a useful suggestion, but it is not that straightforward. If iterator is provided and someone modifies content of said option (e.g. adds new suboption) this iterator may become invalid. Having said that, getOption(int type) is very convenient. It must stay as it is. Added ticket #1296 for iterator work.

pack4()/pack6() - see suggestion about two Option classes, one for V4 and one for V6.

Added to ticket #1295.

What is optionLst_?

A collection of options. Renamed to options_ and added doxygen comment.

Is there any reason why the data members are protected instead of private?

Yes. They are expected to be used in tests.

src/lib/dhcp/option.cc
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).

pack4(): not clear why data_len_+4 bytes are being copied into the output buffer. The type and length have already been copied and the pointer into the buffer advanced. According to the comment in the header file, data_len_ is the length of data only, so copying data_len_+4 risks copying beyond the allocated data area. (Note that only data_len_ bytes are copied in pack6().)

That was a bug. Fixed. The reason why this wasn't caught is that work on v4 is not really started yet. Very small number of v4 functions were implemented to see if the design is feasible. Proper implementation and testing will commence once this ticket is resolved.

len(): need comment as to why the length is increments by the length of the contents of optionLst_. I am assuming that these are suboptions, but there is nothing here or in the header file to indicate that.

Commented how and why specific values are included in len().

valid(): the comments here do not relate to the code.

Removed.

valid(): A simpler statement is return ((universe == V4) || (universe == V6));.

valid(): Under what circumstances (other than wrong universe) can an option be invalid?

This implementation is simple for now, but there is potential for growth here. Note that this is a virtual method, so derived classes may do additional checks, e.g. length of address list type of options may check if their length is divisible by 16. Some checks may be a bit more subtle, e.g. even though FQDN is encoded properly it may still be invalid (e.g. contain a single label).

Now, for the base Option class this method is also expected to be expanded. We may decide to have a table with option types and required lengths (many options). We may expand this into passing message type and then checking if option is allowed to appear in specific message type. The same holds true for suboptions. Some options are allowed to appear only within specific other options etc.

This method will definitely grow.

addOption(): argument should be passed by reference.

No, see above comment.

getOption()/delOption(): should be spaces around the "!=".

Done.

delOption(): the intended result of the method is that after execution, the specified option is not in the collection. If it wasn't there to begin with, after the method call the result is the same, the option is not there. In this case, is it justified to return false? A second question is, if this is an unexpected occurrence, isn't throwing an exception the better way of signalling it?

It's not unexpected. It is faster to try to delete option and check if deletion was successful than search for an option and if it's there, delete it. Both approaches are equally fast in negative case (no option present), but using delOption() is twice as fast in positive case (there's option to be deleted). No exceptions necessary.

src/lib/dhcp/option6_addrlst.h
In the typedef, suggest that AddressContainer is easier to read than AddrsContainer. (The latter only saves two characters.)

Done.

In the constructors, the address(es) should be passed as const.

Done.

In the simplified constructor, the single address should be passed by reference.

Done.

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.

In setAddress(), the argument should be passed by reference and should be const.

Done.

In setAddresses(), the argument should be const.

Done.

src/lib/dhcp/option6_addrlst.cc
Constructor taking a vector: addrs_ can be initialised in the member variable initializer list with addrs_(addrs).

Constructor taking a single IOAddress: addrs_ can be initialized in the member variable initialiser list with addrs_(1, addrs).

That's a nice trick. I didn't know that.

pack()/unpack(): use of "magic number" of 16. Better is to use a named constant.

After couple of minutes of digging around and failing to find already defined constant, I added a new one. Once I find where it is already defined, I'll replace it.

toText(): would suggest that this method just returns the addresses as a simple space-separated list. This will make it easier to use in log messages and other places where a list is required.

toText() is currently used to print out details of incoming and outgoing packets. I agree that shorter form is useful as well. If that is common practice in BIND10 for functions toText() to return short version, so be it. Full text is useful as well. This is generic problem for all classes. Added ticket #1297.

len(): suggest that the DHCPv6 option header length is defined as a constant somewhere.

Added.

Commit-id: 2894b07900ec81ed39284c7d7df5660eb8afbfc0 (part 5 of review changes).

comment:10 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by tomek

Replying to stephen:

src/lib/dhcp/option6_ia.h
Not sure why Option6IA constructor needs a "Universe" parameter. Doesn't this option only exist in V6, in which case there is no need for the Option6IA constructor to have it as an argument; when calling the base constructor, it could pass V6 across explicitly.

True. Removed universe parameter.

Although overridden methods do not need a full Doxygen header, methods declared in this class - even trivial ones - should have such documentation.

Arguments in argument lists should be on the same line where possible (up to a line length of 80).

Done.

Constructor/pack()/unpack(): buf should be passed by reference to avoid a needless copy of the shared_array object.

See responses to previous comments.

src/lib/dhcp/option6_ia.cc
pack(): should use OutputBuffer to take account of field extension.

See responses to previous comments.

pack(): there should be a comment (and possibly an assert()) to ensure that len() returns a value >= 16.

Done.

pack(): when incrementing the pointer, something like "ptr += sizeof(uint32_t)" is more descriptive that "ptr += 4" when coming immediately after the insertion of a uint32_t.

Done.

unpack(): should be spaces around the relational and arithmetic operators.

Done.

toText(): would suggest that this method just returns the addresses as a simple space-separated list. This will make it easier to use in log messages and other places where a list is required. If there is a need to identify the type of this option, then there should be a method that returns a class describing the option type, this class having its own toText() method.

Both short and full text representation are useful. They will be implemented as work on #1297.

src/lib/dhcp/option6_iaaddr.h
Although overridden methods do not need a full Doxygen header, methods declared in this header - even trivial ones - should have such documentation.

Added.

Arguments in argument lists should be on the same line where possible (up to a line length of 80).

Constructor/pack()/unpack(): buf should be passed by reference to avoid a needless copy of the shared_array object.

See previous responses.

setAddress(): argument should be const and passed by reference.

Done.

src/lib/dhcp/option6_iaaddr.cc
Second constructor: when initializing addr_, just pass the string "::" as the argument. As it is, an anonymous IOAddress object is being created then addr_ is being initialized via the copy constructor.

I'm not sure about that. I think I read some time ago that this type of initialization addr_(...) differs from addr_ = ... in constructor code by the addr_ being initialized directly, without copy constructor. I read that long time ago, so I may be wrong. Changed as suggested.

pack(): reference to the magic "16". This should be a symbolic constant.

Added V6ADDRESS_LEN and V4ADDRESS_LEN to asiolink/io_address.h. There are many places throughout the code that may use it.

pack(): same comment as above concerning the incrementing of the ptr object.

Done.

Note: the part of pack() that adds the type and length to the header is common to both Option6IA and Option6IAAddr; presumably it applies to all V6 options. This suggests that it should be a method in the base class that can be called by all derived classes. (It would also mean that the type_ field can remain private to the base class.)

Similarly, the only difference between Option6IA::len() and Option6IAAddr::len() is the size of the header; the rest of the code in the method is concerned with summing the length of the suboptions. This suggests that the code for summing suboption lengths could be put in the base Option class. Better though would be to define Option6Lst as a separate class and provide a len() method within it (as it is returning an attribute of the object).

Yes, as pointed out in already existing TODO in Option6IAAddr::len().

src/lib/dhcp/pkt6.h
Public enums should not end with an underscore. (Trailing underscores are used in private member variables.)

Fixed.

Its is not BIND 10 convention to indent the class definition within a namespace.

Done.

All methods must have Doxygen headers.

Done.

Inconsistent definition of default values in the Pkt6 constructor and setProto (UDP v Pkt6::UDP)

Done.

TODO comment says that member fields should be protected - they should be private.

They should be protected, because they are expected to be accessed from tests that sooner or later will have a class derived fro Pkt6.

Member variables should be commented using Doxygen constructs.

They are now.

addOption(): argument should be passed by reference.

See previous responses.

Ideally inline comments for member variables should line up.

Done.

It's confusing to mix up member variable and member function declarations.

Reordered. Current order is:
public methods
public variables
protected methods
protected variable

src/lib/dhcp/pkt6.cc
The two constructors could be combined into one if default arguments were used.

No, they couldn't. There are 3 cases needed:

  • define specific trans-id (for replying a message)
  • define trans-id to be zero (a subcase of #1, for reconfigure)
  • define random (for transmitting a message)

They could be unified into a single constructor, but it would require inventing a mechanism to indicate if trans-id should be randomized or not (either extra parameter or some special value to passed transid). In some cases only certain parameters may be default. Choosing the most reasonable parameter order would be difficult. It looks simpler the way it is now.

All member variables that are basic types (ints etc.) should be initialized in the constructor, else their value is not defined.

Done.

len(): uses the same code as used in the Option class. See above for comments.

Similar, but different. It calculates length of the packet. For UDP it happens to be the same. For TCP, there is extra overhead.

packUDP(): if the system fails to allocate memory, this is fairly fatal. It seems logical to allow the exception to propagate instead of returning a value as there is little than can be done.

Ok. Removed try and catch.

unpackUDP(): data_ is declared as a shared_array of char, yet the unpack method casts individual bytes to unsigned char. Suggest declaring data_ as a shared_array of uint8_t.

Done.

getType(): this is sufficiently trivial that it could be defined in the header file.

Done.

As there are no virtual functions in the class, there is no need to declare and define a destructor - the default one provided by the compiler will be sufficient.

Removed.

Commit-id: 1213572a61fdf5a81e05fa42b4d7e4ed20c99060 (Part 6 of review changes)

comment:11 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by tomek

Replying to stephen:

src/lib/dhcp/tests/libdhcp_unittest.cc
Spaces required around relational and arithmetic operators and before/after equal signs.

Done.

packOptions6 test: "expected" array is missing comment for opt5.

Added.

unpackOptions6 test: "packed" array is the same as the "expected" array in the previous test. The declaration could be shared between them.

Done.

src/lib/dhcp/tests/option6_addrlst_unittest.cc
basic test: Comment - sampledata, expected1, expected2 and expected3 contain much the same data. Isn't there some way to declare it once and create the arrays dynamically (so avoiding the chance of errors if the code is changed in the future). The same goes for the

text representation of the addresses.
We could generate this on the fly, but the whole point of having defined expected values is to not use any generation mechanisms that may have bugs. The code may change in the future, but the format of DHCPv6 options will not. Tests are not production code. Test code should favor simplicity over efficiency.

Is there a reason to use IPV6 addresses other than 2001:db8::/32 for tests (as required in the coding guidelines). If so, the reason should be documented.

There are specific reasons. I added comments explaining them. I also sent mail to bind10-dev asking for permission to change coding guidelines. 2001:db8::/32 class is for documentation only, NOT for testing.

All tests. Is there a need to declare opt1/2/3 as a pointer to Option6AddrLst and delete them afterwards? As they are being initialized at creation, why not declare them as objects of type Option6AddrLst? It eliminates the need to delete them at the end of the test method. (Alternatively, encapsulate them in a std::auto_ptr to ensure that they are deleted even if the method returns prematurely.)

In tests, I prefer to simplest approach possible. There is nothing that could go wrong with new and delete (except running out of memory, but that would indicate severe memory leak in code).

src/lib/dhcp/tests/option6_ia_unittest.cc
Spaces required around relational and arithmetic operators and before/after equal signs.

Done.

Do the Option6IA objects need to be created via "new"? Can't they be automatically allocated (and so avoid the need for "delete" statements?)

Using new/delete it the simplest way to test whole lifecycle of an abject - creation, use and destruction. Also, object can be destroyed on demand. Added extra EXPECT_NO_THROW around delete instructions.

src/lib/dhcp/tests/option6_iaaddr_unittest.cc
Spaces required around relational and arithmetic operators and before/after equal signs.

Done.

basic test: we are using US conventions in documentation, so the comment by simple_buf[23] should read "3,000,000,000" (i.e. commas, not periods)

Fixed.

There should be a test for len().

Added.

src/lib/dhcp/tests/option_unittest.cc
Do the Option objects need to be created via "new"? Can't they be automatically allocated (and so avoid the need for "delete" statements?)

See above.

src/lib/dhcp/tests/pkt6_unittest.cc
Is there a reason to use IPV6 addresses other than 2001:db8::/32 for tests (as required in the coding guidelines). If so, the reason should be documented.
capture1(): wouldn't it be easier to declare the data in a 98-element array and then memcpy it to pkt->data? (As it is, the columns of the set of assignments do not line up).

This code is auto-generated by packet capture. This is a real-life packet. Also see my post to bind10-dev about improper statements in coding guidelines. The packet capture/generator may be improved as suggested, but that is a very low priority task for now. Added TODO in src/bin/dhcp6/tests/iface_mgr_unittest.cc.

parse_solicit1: as this is a test of unpack(), the test might be better named unpack_solicit1.

Renamed.

No tests for len(), packUPD(), unpackUDP(), getType().

Tests implemented.

Commit-id d3f306a14de315e82d5e18208ddac116af239131 (Part 7 of review changes.

comment:12 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

Replying to stephen:

Review of commit f56892b514cedb0538c37db808ec543d7a4fcfaf - part 2 (cont)

One other thing...

src/lib/asiolink/tests/io_address_unittest.cc
This fails to compile. The from_bytes test includes the line

EXPECT_EQ(addr, IOAddress("192.0.2.3"));

This fails to compile because IOAddress does not support operator<< (used for error messages). It needs to be changed to

EXPECT_TRUE(addr == IOAddress("192.0.2.3"));

src/lib/dhcp/tests/libdhcp_unittest.cc
Fails to compile for a similar reason. Lines of the form:

ASSERT_NE(x, options.end());
EXPECT_EQ(x, options.end());

Need to be changed to

ASSERT_FALSE(x == options.end());
EXPECT_TRUE(x == options.end());

src/lib/dhcp/tests/option6_iaaddr_unittest.cc
Compilation fails with the message:

option6_iaaddr_unittest.cc:79: error: this decimal constant is unsigned only in ISO C90

(The constant in question is 3000000000, which is larger than the maximum signed positive 32-bit integer.)

src/lib/dhcp/tests/option6_ia_unittest.cc
Compilation fails with the message:

option6_ia_unittest.cc: In member function ‘virtual void<unnamed>::Option6IATest_suboptions_unpack_Test::TestBody()’:
option6_ia_unittest.cc:214: error: ‘ia’ may be used uninitialized in this function

src/lib/dhcp/iface_mgr.cc
Fails to compile:

iface_mgr.cc: In member function ‘int isc::IfaceMgr::openSocket(const std::string&, const isc::asiolink::IOAddress&, int)’:
iface_mgr.cc:256: error: dereferencing pointer ‘addr6’ does break strict-aliasing rules
iface_mgr.cc:254: error: dereferencing pointer ‘addr6’ does break strict-aliasing rules
iface_mgr.cc:255: error: dereferencing pointer ‘addr6’ does break strict-aliasing rules
iface_mgr.cc:253: note: initialized from here

(There may be other compilation errors, but I didn't check further.)

I cannot reproduce this problem. It compiles ok on my Ubuntu 11.04 x64 box, with gcc 4.5.2:
g++ (Ubuntu/Linaro? 4.5.2-8ubuntu4) 4.5.2.

make check for dhcp succeeds (currently it fails in DatabaseClientTest? for unrelated reasons).

In my opinion the code is ready for second review.

comment:13 in reply to: ↑ 7 ; follow-up: Changed 8 years ago by stephen

I've re-reviewed the code and have entered my comments as replies to each of the updates you made to the ticket.

Replying to tomek:
src/bin/dhcp6/dhcp6_srv.cc

To check whether rsp holds a valid pointer, the simpler "if (rsp) {" can be used.

I think that is against BIND9 conding standards that is also used in DHCP...

BIND 9 is written in C, so such a check is a test on zero/non-zero (i.e. false/true). BIND 10 uses C++, and the C++0x standard mentions operator unspecified-bool-type() const; which provides a conversion of an object to bool (and so is the correct type for evaluation in a boolean context). We are not using C++0x, but Boost provides such a conversion operation and this conversion is used when checking if a shared_ptr is empty.

src/bin/dhcp6/dhcp6_srv.h
getServerID(): this returns a reference to a shared_ptr within Dhcpv6Srv. But that reference is only valid while Dhcpv6Srv remains in existence. Unless there is a guarantee that Dhcpv6Srv remains in existence while the reference is valid, it should return a shared_ptr by value. (But if Dhcpv6Srv guaranteed to remain in existence, there is no need for serverid_ to be a shared_ptr - it could be a scoped_ptr (avoiding the overhead of reference counting) and Dhcpv6Srv could return a "bare" pointer.)

If a class derived from boost::noncopyable, there is no need to define the copy constructor and assignment operator.

It is not the BIND 10 standard layout to indent code in namespaces.

All Doxygen headers should describe their arguments.

src/bin/dhcp6/iface_mgr.cc

getFillName(): return (name_ + "/"); is simpler than creating a temporary ostringstream object.

No, it is not. It is not possible to add integer to strings.

You're right, I overlooked the ifindex_ variable.

getPlainMac(): Should be spaces around binary operators in the "for" and "if" statements.

In IfaceMgr? constructor, control_buf_ could be declared as a boost::scoped_ptr<> ...

scoped_ptr is not the proper type here ...

My fault, use boost::scoped_array<> instead. (This is better than shared_ptr<> if there is no need to shared the underlying buffer - it avoids the need to manipulate reference counters.)

Can remove the method headers from the .cc file if they are already in the .h file. The typo "nad" has been moved to the .h file :-)

src/bin/dhcp6/iface_mgr.h
Same comment as before about the indentation with respect to namespaces.

send() should take a reference to a shared pointer as its argument. Also, if the argument is unchanged, it should be const.

I prefer to keep it as separate copy of shared pointer. We may decide to use async send sometime later.

If async send is used, we can always copy the pointer if we need to. Ignoring possible compiler optimisation, this would mean two copies: one copy when the shared_ptr is passed by value to the method, and one copy of the method's argument into the location where the pointer is stored for the duration of the asynchronous call. Passing the shared_ptr by reference eliminates the first copy.

src/bin/dhcp6/tests/iface_mgr_unittest.cc

I politely refuse to implement this

:-) As the code is only temporary, leave it for now.

Last edited 8 years ago by stephen (previous) (diff)

comment:14 in reply to: ↑ 8 ; follow-up: Changed 8 years ago by stephen

Replying to tomek:

src/lib/dhcp/dhcp6.h
Seems to be a mis-alignment of the values for options

That file is a verbatim import from ISC DHCP...

OK, I hadn't realised them. No changes needed (although leave in the ones you have done).

Putting the declarations into a enum also allows...

That would be useful. Having options named exactly the same way as in RFC would be nice. Unfortunately, that means that there will be a conflict in ISC DHCP.

Do what you need to do for now. In the longer-term, we need to decide how to handle the compatibility between Kea and DHCPv4.

src/lib/dhcp/libdhcp.h
The idea of a "LibDHCP" class is not really object oriented - it seems to imply a general collection of loosely-related functions, which is more of a C paradigm. What we have in this class seems more to be an OptionCollection object - an object capable of disassembling and assembling

Related to the above, there is are packOptions6() and unpackOptions6() methods and an OptionFactoryRegister() method that requires an indication of the "universe" in which it operates. All of these seem to be arguing for two classes, one for v6 and one for v4, each with pack, unpack and factoryRegister methods.

That is a work in progress. The idea is to

I think the rest of the comment got lost :-). But we need to look at the design to see if "LibDHCP" is really a natural object.

src/lib/dhcp/libdhcp.cc
unpackOptions6(): Using the InputBuffer class in "util" allows items of data to be read from a buffer in 16-bit words.

InputBuffer is unsuitable here. Hooks may modify incoming packet, so input buffer is actually read-write.

Fair point, but I think encapsulating the input data in some object that simplifies the packing/unpacking of data in it is worth some investigation at some time in the future.

unpackOptions6(): Should be spaces around relational operators (in "while (offset<end)").

Being pedantic, it's not usual in BIND 10 code to put spaces after an opening parenthesis or before a closing one. (We tend to obey the Google guidelines here: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Horizontal_Whitespace)

comment:15 in reply to: ↑ 9 ; follow-up: Changed 8 years ago by stephen

Replying to tomek:
src/lib/dhcp/option.h

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

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

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.

comment:16 in reply to: ↑ 10 ; follow-up: Changed 8 years ago by stephen

Replying to tomek:

Only comment here is:

src/lib/dhcp/pkt6.h
Inconsistent definition of default values in the Pkt6 constructor and setProto (UDP v Pkt6::UDP)

Done

Just noticed that although setProto() is declared in the .h file, there is no definition of it anywhere.

comment:17 in reply to: ↑ 11 ; follow-up: Changed 8 years ago by stephen

Replying to tomek:

src/lib/dhcp/tests/option6_addrlst_unittest.cc
All tests. Is there a need to declare opt1/2/3 as a pointer to Option6AddrLst and delete them afterwards? As they are being initialized at creation, why not declare them as objects of type Option6AddrLst? It eliminates the need to delete them at the end of the test method. (Alternatively, encapsulate them in a std::auto_ptr to ensure that they are deleted even if the method returns prematurely.)

In tests, I prefer to simplest approach possible. There is nothing that could go wrong with new and delete (except running out of memory, but that would indicate severe memory leak in code).

Not sure I follow the logic here. I would have thought that declaring them as a local variable (as opposed to a pointer) would be easier. However, your reply later on in the comment about using new/delete to test the full lifecycle makes sense. I would suggest that we add EXPECT_NO_THROW round the construction/deletion to be complete.

BTW, we still have declarations of the form:

Option6AddrLst * opt

The space before the "*" should be removed.

comment:18 in reply to: ↑ 12 Changed 8 years ago by stephen

Replying to tomek:

src/lib/asiolink/tests/io_address_unittest.cc
The addition of operator const char*() has fixed this compilation problem.

src/lib/dhcp/tests/libdhcp_unittest.cc
I still get failures with lines like:

ASSERT_NE(x, options.end());
EXPECT_EQ(x, options.end());

... as the type isc::dhcp::Option::Option6Collection::const_iterator has no conversion to a text string. As noted, replacing them by:

ASSERT_FALSE(x == options.end());
EXPECT_TRUE(x == options.end());

... compiles. I've made these changes to the file and pushed them to the repository.

src/lib/dhcp/tests/option6_iaaddr_unittest.cc
Compilation fails with the message:

option6_iaaddr_unittest.cc:79: error: this decimal constant is unsigned only in ISO C90

Replacing 3000000000 with 3000000000U has removed the warning and solved the problem.

src/lib/dhcp/iface_mgr.cc
Fails to compile:

I don't get this problem now.

currently it fails in DatabaseClientTest for unrelated reasons

I also get that - it will probably go away when the branch is merged into master.

Note that I've updated two files in this branch and pushed them to the repository; you will need to pull them before you do anything else.

comment:19 Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

I've reviewed the changes: my responses can be found in comments 13 through 18. I suggest that we discuss the next steps with Shane before doing anything further.

comment:20 in reply to: ↑ 13 Changed 8 years ago by tomek

Replying to stephen:

To check whether rsp holds a valid pointer, the simpler "if (rsp) {" can be used.

I think that is against BIND9 conding standards that is also used in DHCP...

BIND 9 is written in C, so such a check is a test on zero/non-zero (i.e. false/true). BIND 10 uses C++, and the C++0x standard mentions operator unspecified-bool-type() const; which provides a conversion of an object to bool (and so is the correct type for evaluation in a boolean context). We are not using C++0x, but Boost provides such a conversion operation and this conversion is used when checking if a shared_ptr is empty.

Ok. I think the code uses simple "if (pointer) {" code. If I missed anything, please do let me know.

src/bin/dhcp6/dhcp6_srv.h
getServerID(): this returns a reference to a shared_ptr within Dhcpv6Srv. But that reference is only valid while Dhcpv6Srv remains in existence. Unless there is a guarantee that Dhcpv6Srv remains in existence while the reference is valid, it should return a shared_ptr by value. (But if Dhcpv6Srv guaranteed to remain in existence, there is no need for serverid_ to be a shared_ptr - it could be a scoped_ptr (avoiding the overhead of reference counting) and Dhcpv6Srv could return a "bare" pointer.)

Fixed.

If a class derived from boost::noncopyable, there is no need to define the copy constructor and assignment operator.

Copy constructor and assignment operator removed.

It is not the BIND 10 standard layout to indent code in namespaces.

Fixed for dhcp6_srv.h and iface_mgr.h. Code in src/lib/dhcp seems to follow this guideline already. Also note that there is nothing in CodingGuidelines about indent (or lack of thereof) regarding namespaces. I'll ask on jabber and update CodingGuidelines if I get approval.

All Doxygen headers should describe their arguments.

Added descriptions for missing arguments.

src/bin/dhcp6/iface_mgr.cc
getPlainMac(): Should be spaces around binary operators in the "for" and "if" statements.

Done.

In IfaceMgr? constructor, control_buf_ could be declared as a boost::scoped_ptr<> ...

scoped_ptr is not the proper type here ...

My fault, use boost::scoped_array<> instead. (This is better than shared_ptr<> if there is no need to shared the underlying buffer - it avoids the need to manipulate reference counters.)

I did as you asked, but I don't like it. We gained nothing (this buffer was freed correctly), but code is more complicated now (need to use &control_buf[0] instead of simpler control_buf_) and is more difficult to debug (gcc can't display scoped_array<char> as easily as char *). That is unfortunate, as code around sendmsg and recvmsg are murky enough.

Can remove the method headers from the .cc file if they are already in the .h file. The typo "nad" has been moved to the .h file :-)

Redundant headers removed.

src/bin/dhcp6/iface_mgr.h
Same comment as before about the indentation with respect to namespaces.

Fixed.

send() should take a reference to a shared pointer as its argument. Also, if the argument is unchanged, it should be const.

I prefer to keep it as separate copy of shared pointer. We may decide to use async send sometime later.

If async send is used, we can always copy the pointer if we need to. Ignoring possible compiler optimisation, this would mean two copies: one copy when the shared_ptr is passed by value to the method, and one copy of the method's argument into the location where the pointer is stored for the duration of the asynchronous call. Passing the shared_ptr by reference eliminates the first copy.

Ok. Done.

Commit-ids: d8d0a731ce43167d98b7c7e855b8a31b9deacdfb, cb59e65da8d1c9498d1dc8845e277567814fe400 and a7605f27d5575c6fbeb325705220039936c81e53.

comment:21 in reply to: ↑ 14 Changed 8 years ago by tomek

Replying to stephen:

Replying to tomek:

Putting the declarations into a enum also allows...

That would be useful. Having options named exactly the same way as in RFC would be nice. Unfortunately, that means that there will be a conflict in ISC DHCP.

Do what you need to do for now. In the longer-term, we need to decide how to handle the compatibility between Kea and DHCPv4.


src/lib/dhcp/libdhcp.h
The idea of a "LibDHCP" class is not really object oriented - it seems to imply a general collection of loosely-related functions, which is more of a C paradigm. What we have in this class seems more to be an OptionCollection object - an object capable of disassembling and assembling

Related to the above, there is are packOptions6() and unpackOptions6() methods and an OptionFactoryRegister() method that requires an indication of the "universe" in which it operates. All of these seem to be arguing for two classes, one for v6 and one for v4, each with pack, unpack and factoryRegister methods.

That is a work in progress. The idea is to

I think the rest of the comment got lost :-). But we need to look at the design to see if "LibDHCP" is really a natural object.

There's ticket 1303 for that.

src/lib/dhcp/libdhcp.cc
unpackOptions6(): Using the InputBuffer class in "util" allows items of data to be read from a buffer in 16-bit words.

InputBuffer is unsuitable here. Hooks may modify incoming packet, so input buffer is actually read-write.

Fair point, but I think encapsulating the input data in some object that simplifies the packing/unpacking of data in it is worth some investigation at some time in the future.

The idea here is that Pkt6 serves such purpose. We may need to look at unifying OutputBuffer? and Pkt6 eventually.

unpackOptions6(): Should be spaces around relational operators (in "while (offset<end)").

Being pedantic, it's not usual in BIND 10 code to put spaces after an opening parenthesis or before a closing one. (We tend to obey the Google guidelines here: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Horizontal_Whitespace)

Fixed, but this link should be added to CodingGuidelines.

There will be single commit with this and following comments.

comment:22 in reply to: ↑ 15 Changed 8 years ago by tomek

Replying to stephen:

Replying to tomek:
src/lib/dhcp/option.h

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.

I changed to pass by reference in many places. The one notable exception is addOption(). If parameter is passed by value, it is possible to pass shared_ptr to derieved classes. With pass by reference it is not possible and required cumbersome casting every time this method is used. Therefore in this case pass be value stays.

src/lib/dhcp/option.cc

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.

That is something that we should pursue. Added ticket 1312 to get back to this topic.

src/lib/dhcp/option6_addrlst.h

In the constructor for parsing the received option (and in both pack() and unpack()), "buf" should be passed by reference.

Done. Also changed other pack/unpack methods in other options.

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.

Cool functions. Started using them, also added ticket #1313 for writing writeUint32 and readUint32 functions.

Commit-id: 40249e03304a26d147f83fe1899a7fc8207c70f0

comment:23 in reply to: ↑ 16 Changed 8 years ago by tomek

Replying to stephen:

Just noticed that although setProto() is declared in the .h file, there is no definition of it anywhere.

Added implementation.

comment:24 in reply to: ↑ 17 Changed 8 years ago by tomek

Replying to stephen:

Replying to tomek:

src/lib/dhcp/tests/option6_addrlst_unittest.cc
All tests. Is there a need to declare opt1/2/3 as a pointer to Option6AddrLst and delete them afterwards? As they are being initialized at creation, why not declare them as objects of type Option6AddrLst? It eliminates the need to delete them at the end of the test method. (Alternatively, encapsulate them in a std::auto_ptr to ensure that they are deleted even if the method returns prematurely.)

In tests, I prefer to simplest approach possible. There is nothing that could go wrong with new and delete (except running out of memory, but that would indicate severe memory leak in code).

Not sure I follow the logic here. I would have thought that declaring them as a local variable (as opposed to a pointer) would be easier. However, your reply later on in the comment about using new/delete to test the full lifecycle makes sense. I would suggest that we add EXPECT_NO_THROW round the construction/deletion to be complete.

Added EXPECT_NO_THROW.

BTW, we still have declarations of the form:

Option6AddrLst * opt

The space before the "*" should be removed.

Removed unnecessary spaces.

comment:25 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

All issues pointed out is second round of reviews addressed. ChangeLog? updated. Changes pushed. Ready for round 3.

comment:26 Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

All OK, you can go ahead and merge now.

comment:27 Changed 8 years ago by tomek

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged to master, pushed, resolved minor problems with NetBSD and Solaris.

Closing ticket.

Note: See TracTickets for help on using tickets.