Opened 8 years ago

Closed 8 years ago

#1540 closed enhancement (fixed)

dhcp code refactor: Pkt6 and Options for DHCPv6

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

Description (last modified by tomek)

To meet end of 2011 deadline, some features had to be implemented in less than optimal mannter. This ticket will contains assorted fixes, small improvements and general code refactoring.

  1. Pkt6 and DHCPv6 Options should use the same approach as Pkt4 and DHCPv4 options (i.e. don't use shared_array<uint8_t>, but rather vector<uint8_t>, possibly with shared_ptr<> if needed.
  1. Define types for commonly used pointers.

Subtickets

Change History (20)

comment:1 Changed 8 years ago by tomek

  • Owner set to tomek
  • Status changed from new to accepted

comment:2 Changed 8 years ago by tomek

  • Description modified (diff)
  • Summary changed from dhcp code refactor/clean-up, part 1 to dhcp code refactor: Pkt6 and Options for DHCPv6

comment:3 Changed 8 years ago by stephen

  • Milestone changed from DHCP 2012 to Sprint-DHCP-20120206

comment:4 Changed 8 years ago by tomek

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

Changes checked in.

Friendly warning: changes are rather simple, but extensive (29 files modified).

Please review.

Note: Compilation may fail if you are using automake 1.11.2 (that what happened to my on my Mac OS). That is a automake bug, see http://lists.gnu.org/archive/html/automake/2012-01/msg00002.html

comment:5 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to stephen

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

  • Owner changed from stephen to tomek

General
The use of PktXPtr makes the code clearer, but we should also have a ConstPktXPtr data type - a pointer to a const object. (This approach is used elsewhere in BIND 10, e.g. RRsetPtr and ConstRRsetPtr.) Note that this also applies to OptionPtr as well.

Although this review will mention places where a ConstPktXPtr can be used, those comments can be ignored for now: it is suggested that their introduction be deferred to a separate ticket - introducing "const" is likely to require a lot of changes.

src/bin/dhcp4/dhcp4_srv.{cc,h}
Dhcp4Srv::run() - query should be declared and initialised on one line.

Dhcp4Srv::run() - query could probably be a ConstPkt4Ptr, in which case a number of the processXxxx methods need to be modified to accept a ConstPkt4Ptr. (In fact, the argument to these is likely to be of data type const ConstPkt4Ptr& - a const pointer to a const object.)

Dhcp4Srv::copyDefaultFields() - "question" argument should be const ConstPkt4Ptr&.

Many methods - Pointers to options are created. It is likely that these could be const options, i.e. ConstOptionPtrs.

src/bin/dhcp4/dhcp6_srv.{cc,h}
Comments made above about const pointers apply.

Dhcp6Srv::run() - query should be declared and initialised on one line.

Dhcp6Srv::setServerID() - check the Ethernet type (you're not in a hotel now :-)

Dhcp6Srv::setServerID() - for loop: spaces around binary operators please.

Dhcp6Srv::processRequest()/processSolicit() - why the blank line before the creation of "reply" - the remainder of the processXxxx() methods do not have it.

Dhcp6Srv::processRebind(): Creation of "reply" does not need to be split across multiple lines.

src/bin/dhcp6/tests/dhcp6_srv_unittest.{cc,h}
Solicit_basic test - the "for" loop at the head of the function should included braces around the (single) statement.

Solicit_basic test - in the initialization of "clientid", there should be spaces around the "+".

src/lib/dhcp/iface_mgr.cc
IfaceMgr::send() - (V4 version) Shouldn't the argument have a data type of Pkt4Ptr?

IfaceMgr::send() - (V4 version) a number of the local variables could be declared closer to where they are first used: in the case of "cmsg" and "result", the declaration could be combined with initialization.

IfaceMgr::send() - (V4 version) although the compiler apparently accepts it for the purposes of calculating a sizeof(), it feels uncomfortable to dereference pktinfo before initializing it. Instead of "sizeof(*pktinfo)", "sizeof(in6_pktinfo)" seems safer. Or swap this statement with the one before it?

IfaceMgr::send() - (V4 version) Should use "static_cast<in6_pktinfo*>" (instead of the C-style cast) to set pktinfo.

IfaceMgr::send() - (V6 version) Shouldn't the argument have a data type of Pkt6Ptr?

IfaceMgr::send() - (V6 version) Use a "static_int<char*>" to cast the data for v.iov_base instead of the C-style cast.

IfaceMgr::send() - (V6 version) Remove the comment about ticket #1237. Also, can the Linux-specific part be put into a separate function (possibly in a separate file)? It would serve to isolate the Linux-specific code from the rest of the function.

IfaceMgr::receive4() - The "while" loop looks awkward with two "++s" increments. Converting to "for" loop and inverting the test of the address family would simplify it. Also, as this appears to be separate from the rest of the code, can't it be placed into a separate method?

IfaceMgr::receive4() - a lot of the declarations look similar to that in other methods - is there anything that could be made common?

IfaceMgr::receive4() - "pkt" should have the data type Pkt4Ptr (and should be declarated where it is initialized).

IfaceMgr::receive4() - return variable should be of type Pkt4Ptr.

IfaceMgr::receive4() - same comments made above concerning declaraing variables closer to their use apply.

IfaceMgr::receive4() - no need for a blank line after calling recvmsg() and the test on result.

IfaceMgr::receive4() - suggest place the Linux-specific code in a separate function (in a separate file).

IfaceMgr::receive6() - most of the comments made for receive4() (including the conversion of the loop over the sockets from "while" to "for", and variable initialization) apply here.

IfaceMgr::receive6() - the "no interfaces detected" message should be made only if "ifaces_.empty()" is true. Currently it can be thrown if no interface has IPV6 capability (which is somewhat different).

IfaceMgr::receive6() - casts in the calls to setXxxxAddr should be C++-style.

IfaceMgr::getSocket() - in the method signature, "const Pkt6&" appears to be the more usual style than "Pkt6 const&".

IfaceMgr::getSocket() - (both versions) in the "for" loop, as the conditions aren't too complicated, inverting and combining the conditions in a single "if" test and placing the "return" in the body avoids the need for "continue" statements.

src/lib/dhcp/libdhcp++.{cc,h}
unpackOptions6() - should not be a space between "end" and ")" in the first "if" test.

unpackOptions4(): using pre-increments is generally better than post-increments.

unpackOptions6()/unpackOptions4() - in the call to options.insert(), you could use std::make_pair as a less-verbose way of creating a pair to insert into "options".

OptionFactoryRegister() - should be no space preceding the "*" in the factory type. Spaces needed around "!=" in the "if" statements.

OptionFactoryRegister() - throws an exception if the option type is greater than 254 - although "end" option has a value of 255. If "end" can never be passed to this method, there should be a comment explaining why.

src/lib/dhcp/option.h
Typedef of OptionBufferPtr does not need spaces after "<" and before ">".

Typedef of OptionCollection does not need to be split across two lines. Also, no need for a space before the ">".

A comment should be added to the effect that it is assumed all over the code that OptionBuffer is a vector. (In several places an iterator over OptionBuffer is dereferenced and assumed to point to a set of contiguous bytes that are interpreted as a data type such as uint16_t. If the underlying OptionBuffer data type were changed, this might not be true.)

General: revisiting the Option class, I was struck by the separation of functions depending on "Universe". What is the argument against an Option (abstract) base class with Option4 and Option6 derived classes?

src/lib/dhcp/option.cc
pack4() - since the constructor only rejects the V4 option type if type is greater than 255, there is a possibility that the Option object could hold an "end" option. In this case, pack4() will attempt to add a length byte after it.

pack4() - should add a sanity check for option size when writing the length byte for a V4 option - len() returns a uint16_t so could potentially overflow the field.

pack() - "return" should have the argument enclosed in brackets.

getOption()/delOption() - argument type should be uint16_t, not "unsigned short".

addOption() - consider use of make_pair().

src/lib/dhcp/option6_addrlst.cc
Remove comment "this wrapping is *ugly*..." ... unless you think it is still ugly, in which case complete the comment :-)

Option6AddrLst::unpack() - use V6ADDRESS_LEN in the "if" statement instead of 16. (More informative and consistent with use later on in the method.)

Option6AddrLst::unpack() - would prefer the result of "distance % 16" to be explicitly compared against 0 (instead of using the implicit conversion to bool): the check is whether the remainder is non-zero and an explicit numerical check reinforces that.

src/lib/dhcp/option6_ia.cc
Option6IA::unpack() - there is a warning if the distance between the start and end iterators is less than 12; what if it is greater than 12? Also, could this value be a symbolic constant (c.f. OPTION6_IAADR_LEN in Option6IAAddr::unpack())

src/lib/dhcp/pkt4.h
The comments for the member variable data_ use double slashes instead of the triple slashes required by Doxygen and used for other member variables.

src/lib/dhcp/pkt6.h
getBuffer() header - should say "This buffer is only valid while the Pkt4 object is valid."

len() header - the header comment is not clear. The "brief" explanation is that it returns the size of the packet, but the text implies that it returns the length of the options fields - and treats the header as one of them. Also, why does options_ have to be set - if it is clear doesn't it mean that the packet has no options?

A consistent style should be used for the inline expansions of one-line methods. In some cases the expansion is on the same line as the declaration: in other cases it is split across three lines.

Does ifindex_ deserve the "brief" line in its explanatory comments?

There are apparently two ways of identifying the interface - the name and the index. Are these truly independent or is there a unified way of accessing the relevant interface?

No need for spaces after the opening parenthesis and beflore the closing parenthesis in some EXPECT_EQ checks.

src/lib/dhcp/tests/iface_mgr_unittest.cc
sendReceive6 test - the loop setting the dummy payload should have spaces around binary operators.

src/lib/dhcp/tests/libdhcp++_unittest.cc
packOptions4 and packOptions6 tests - no need for the space before the ">" in the "opts.insert" lines.

packOptions6 test - the EXPECT_NO_THROW test can be on a single line.

No need for spaces after the opening parenthesis and before the closing parenthesis in some EXPECT_EQ checks.

src/lib/dhcp/tests/option6_addrlst_unittest.cc
No need for spaces after the opening parenthesis and before the closing parenthesis in some EXPECT_EQ checks.

src/lib/dhcp/tests/option6_ia_unittest.cc
suboptions_pack test - "sub1" declaration does not need to sbe split across two lines.

suboptions_unpack test - not sure what the cryptic comment "48 bytes" means.

suboptions_unpack test - in the data declaration, binary operators should have a space around them.

src/lib/dhcp/tests/option6_iaaddr_unittest.cc
basic test - in the initialization loop, binary operators should have a space around them.

src/lib/dhcp/tests/option_unittest.cc
No need for spaces after the opening parenthesis and before the closing parenthesis in some EXPECT_EQ checks.

src/lib/dhcp/tests/pkt6_unittest.cc
constructor test - No space before the "*" in the declaration of pkt1.

No need for spaces after the opening parenthesis and before the closing parenthesis in some EXPECT_EQ checks.

capture() - C++ style is to put the "*" after the return type, not before the function name.

capture() - can we line up the assignment of data elements? However, it would be clearer to statically initialize the array.

packUnpack test - "3*Option" - should be spaces around the "*".

addGetDelOptions test - C++ style is to put the "*" after the return type, not before the function name (declaration of "parent".) Also should be no space before the closing parenthesis.

src/lib/util/buffer.h
The change appears OK, but tests/buffer_unittest.cc should be extended to check that the constructor using a vector works.

ChangeLog
Text for the entry is OK.

comment:7 in reply to: ↑ 6 Changed 8 years ago by tomek

Replying to stephen:

General
The use of PktXPtr makes the code clearer, but we should also have a ConstPktXPtr data type - a pointer to a const object. (This approach is used elsewhere in BIND 10, e.g. RRsetPtr and ConstRRsetPtr.) Note that this also applies to OptionPtr as well.

That is true, but actual usefulness of this in case of DHCP would be very limited. There's good reason for that. Once hooks are implemented, callouts code can modify practically any part of packet or option at any stage. So can't really say "from now on this object is read-only". We can use const OptionPtr? though (i.e. make a promise that specific function will not modify the pointer).

src/bin/dhcp4/dhcp4_srv.{cc,h}
Dhcp4Srv::run() - query should be declared and initialised on one line.

Done.

src/bin/dhcp4/dhcp6_srv.{cc,h}
Comments made above about const pointers apply.

Dhcp6Srv::run() - query should be declared and initialised on one line.

Done.

Dhcp6Srv::setServerID() - check the Ethernet type (you're not in a hotel now :-)
Dhcp6Srv::setServerID() - for loop: spaces around binary operators please.

Actually written this method for real. Now it tries to find suitable interface to generate DUID and generates proper DUID-LLT (that is recommended type). If that fails (running on OS that does not have interface detection implemented or on a box with really weird interfaces), it falls back to DUID-EN and uses ISC's enterprise number + random values.

Dhcp6Srv::processRebind(): Creation of "reply" does not need to be split across multiple lines.

Done. Also marked all skeleton methods with appropriate TODO.

src/bin/dhcp6/tests/dhcp6_srv_unittest.{cc,h}
Solicit_basic test - the "for" loop at the head of the function should included braces around the (single) statement.

Solicit_basic test - in the initialization of "clientid", there should be spaces around the "+".

Done.

src/lib/dhcp/iface_mgr.cc
IfaceMgr::send() - (V4 version) Shouldn't the argument have a data type of Pkt4Ptr?

IfaceMgr::send() - (V4 version) a number of the local variables could be declared closer to where they are first used: in the case of "cmsg" and "result", the declaration could be combined with initialization.

Done.

IfaceMgr::send() - (V4 version) although the compiler apparently accepts it for the purposes of calculating a sizeof(), it feels uncomfortable to dereference pktinfo before initializing it. Instead of "sizeof(*pktinfo)", "sizeof(in6_pktinfo)" seems safer. Or swap this statement with the one before it?

Done.

IfaceMgr::send() - (V4 version) Should use "static_cast<in6_pktinfo*>" (instead of the C-style cast) to set pktinfo.

IfaceMgr::send() - (V6 version) Shouldn't the argument have a data type of Pkt6Ptr?

Done.

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

Replying to stephen:

IfaceMgr::send() - (V6 version) Use a "static_int<char*>" to cast the data for v.iov_base instead of the C-style cast.

That's not that simple. getData() returns const void* while iov_base structure has field of type void*. static_cast from const void* to void* does not work. I used const_cast<void*>.

Using C++ style casts while dealing with system API is awkward. Around line 635 there is following conversion:

struct in_pktinfo* pktinfo =(struct in_pktinfo *)CMSG_DATA(cmsg);

CMSG_DATA is a macro defined in system headers. On my Linux box, it is defined as
((unsigned char *) ((struct cmsghdr *) (cmsg) + 1))
so CMSG_DATA() is of unsigned char * type. The only way to cast that to struct in_pktinfo* is to use reinterpret_cast, which is only slightly better than pure C cast. I would be less readable, though. I left C style cast for now. Let me know if you prefer reinterpret_cast instead.

IfaceMgr::send() - (V6 version) Remove the comment about ticket #1237. Also, can the Linux-specific part be put into a separate function (possibly in a separate file)? It would serve to isolate the Linux-specific code from the rest of the function.

We can do this, but I would very much like to avoid that. It would require putting msghdr and iovec structures as parameters. I'm not sure if those are defined in the same way on all supported systems (and if they are defined at all). That method would be mentioned in iface_mgr.h that is frequently used in many places. Instead of separating Linux code, we would spread it all around. The other alternative would be to pass msghdr and iovec pointers as void*, but that would be an ugly kludge.

IfaceMgr::receive4() - The "while" loop looks awkward with two "++s" increments. Converting to "for" loop and inverting the test of the address family would simplify it. Also, as this appears to be separate from the rest of the code, can't it be placed into a separate method?

This code will be removed when support for receiving data on more than one interface (using select()) will be implemented. Changed to for loop. Added comment with reference to ticket #1555.

IfaceMgr::receive4() - a lot of the declarations look similar to that in other methods - is there anything that could be made common?

Again, creating functions for them would require using some possibly Linux specific structures in header.

We could define a function (not a member of the IfaceMgr? class) that would have a scope limited to iface_mgr.cc, but that would be breaking down OO paradigm (and a good coding) for just a minor benefit.

IfaceMgr::receive4() - "pkt" should have the data type Pkt4Ptr (and should be declarated where it is initialized).

IfaceMgr::receive4() - return variable should be of type Pkt4Ptr.

Done.

IfaceMgr::receive4() - same comments made above concerning declaraing variables closer to their use apply.

Done.

IfaceMgr::receive4() - no need for a blank line after calling recvmsg() and the test on result.

IfaceMgr::receive4() - suggest place the Linux-specific code in a separate function (in a separate file).

See above regarding polluting headers with Linux-specific structures.

IfaceMgr::receive6() - most of the comments made for receive4() (including the conversion of the loop over the sockets from "while" to "for", and variable initialization) apply here.

IfaceMgr::receive6() - the "no interfaces detected" message should be made only if "ifaces_.empty()" is true. Currently it can be thrown if no interface has IPV6 capability (which is somewhat different).

Updated the message.

IfaceMgr::receive6() - casts in the calls to setXxxxAddr should be C++-style.

I think reinterpret_cast is the only suitable cast method.

IfaceMgr::getSocket() - in the method signature, "const Pkt6&" appears to be the more usual style than "Pkt6 const&".

IfaceMgr::getSocket() - (both versions) in the "for" loop, as the conditions aren't too complicated, inverting and combining the conditions in a single "if" test and placing the "return" in the body avoids the need for "continue" statements.

Done.

src/lib/dhcp/libdhcp++.{cc,h}
unpackOptions6() - should not be a space between "end" and ")" in the first "if" test.

unpackOptions4(): using pre-increments is generally better than post-increments.

That is true regarding objects. However, unpackOptions4 post-increments only plain integers, so there is no difference in performance. Increasing offset after byte was consumer is also more natural.

unpackOptions6()/unpackOptions4() - in the call to options.insert(), you could use std::make_pair as a less-verbose way of creating a pair to insert into "options".

Great trick! I was not aware of the make_pair.

OptionFactoryRegister() - should be no space preceding the "*" in the factory type. Spaces needed around "!=" in the "if" statements.

OptionFactoryRegister() - throws an exception if the option type is greater than 254 - although "end" option has a value of 255. If "end" can never be passed to this method, there should be a comment explaining why.

Added appropriate comment. Also added exception in case of another special value 0 (that's padding option).


src/lib/dhcp/option.h
Typedef of OptionBufferPtr does not need spaces after "<" and before ">".

Typedef of OptionCollection does not need to be split across two lines. Also, no need for a space before the ">".

A comment should be added to the effect that it is assumed all over the code that OptionBuffer is a vector. (In several places an iterator over OptionBuffer is dereferenced and assumed to point to a set of contiguous bytes that are interpreted as a data type such as uint16_t. If the underlying OptionBuffer data type were changed, this might not be true.)

Added appropriate comment.

General: revisiting the Option class, I was struck by the separation of functions depending on "Universe". What is the argument against an Option (abstract) base class with Option4 and Option6 derived classes?

Hierarchy bloat and code duplication. Many soon to be implemented options, like integer arrays are shared between v4 and v6. Implementing them separate would need to code the same methods twice (or use ugly multiple inheritance).

src/lib/dhcp/option.cc
pack4() - since the constructor only rejects the V4 option type if type is greater than 255, there is a possibility that the Option object could hold an "end" option. In this case, pack4() will attempt to add a length byte after it.

Good catch. Also added check that 0 (PAD option) is invalid as well.

pack4() - should add a sanity check for option size when writing the length byte for a V4 option - len() returns a uint16_t so could potentially overflow the field.

Updated check in pack4()

pack() - "return" should have the argument enclosed in brackets.

getOption()/delOption() - argument type should be uint16_t, not "unsigned short".

Done.

addOption() - consider use of make_pair().

Done.

src/lib/dhcp/option6_addrlst.cc
Remove comment "this wrapping is *ugly*..." ... unless you think it is still ugly, in which case complete the comment :-)

My sense of good taste (or lack of thereof) must have changed recently. I don't consider
calling 4 methods (getAddress().to_v6().to_bytes().data()) to get an actual value of address from address object ugly anymore.

Option6AddrLst::unpack() - use V6ADDRESS_LEN in the "if" statement instead of 16. (More informative and consistent with use later on in the method.)

Option6AddrLst::unpack() - would prefer the result of "distance % 16" to be explicitly compared against 0 (instead of using the implicit conversion to bool): the check is whether the remainder is non-zero and an explicit numerical check reinforces that.

Done.

src/lib/dhcp/option6_ia.cc
Option6IA::unpack() - there is a warning if the distance between the start and end iterators is less than 12; what if it is greater than 12? Also, could this value be a symbolic constant (c.f. OPTION6_IAADR_LEN in Option6IAAddr::unpack())

Nothing wrong happens if it is greater than 12. That only means that sub-options are
present (which is usually the case for IA_NA and IA_PD as they are containers for addresses and prefixes). Distance equal 12 means no sub-options.

src/lib/dhcp/pkt4.h
The comments for the member variable data_ use double slashes instead of the triple slashes required by Doxygen and used for other member variables.

Fixed.

src/lib/dhcp/pkt6.h
getBuffer() header - should say "This buffer is only valid while the Pkt4 object is valid."

It already said that. I think you meant "Pkt6" :-) Fixed.

len() header - the header comment is not clear. The "brief" explanation is that it returns the size of the packet, but the text implies that it returns the length of the options fields - and treats the header as one of them. Also, why does options_ have to be set - if it is clear doesn't it mean that the packet has no options?

Comment clarified.

A consistent style should be used for the inline expansions of one-line methods. In some cases the expansion is on the same line as the declaration: in other cases it is split across three lines.

I think I fixed them all.

Does ifindex_ deserve the "brief" line in its explanatory comments?

I don't understand this comment. Do you want me to remove "@brief interface index" from
description of int ifindex_ field? I wanted it to be there to explain (even when browsing
Doxygen documentation for the whole class that shows only brief descriptions) that ifindex
stands for "interface index", not conditional index ("if index").

There are apparently two ways of identifying the interface - the name and the index. Are these truly independent or is there a unified way of accessing the relevant interface?

Keeping them separated is really the way to go. Usually they are equally unique, but in some
cases ifindex can change. It was the case in some older Ubuntu boxes that had race some kind of race condition. Also, in some systems (Windows XP) interface names can be really weird, like "Połączenie sieci bezprzewodowej" (pol. Wireless network link). Note spaces and national characters. It is also unfortunate that those names are language specific and change between languages. In such bizarre environments it is better to rely on interface indexes. There are also cases when interface can disappear and reappear (e.g. any USB interface) and there is no guarantee that it will get the same index. There are valid use cases, when we also want to refer non-existing interface by its name, e.g. when CPE device boots and wifi link initialization takes a long time. Also, depending on the context it is more convenient to use one identification or the other. When dealing with configuration, it is more likely that we will use interface names. On the other hand, socket operations more likely will use ifindex to refer specific interface.

The bottom line is that we need both.

No need for spaces after the opening parenthesis and beflore the closing parenthesis in some EXPECT_EQ checks.

I removed several instances of unnecessary spaces in various tests. I hope I found them all.

src/lib/dhcp/tests/iface_mgr_unittest.cc
sendReceive6 test - the loop setting the dummy payload should have spaces around binary operators.

Done.

src/lib/dhcp/tests/libdhcp++_unittest.cc
packOptions4 and packOptions6 tests - no need for the space before the ">" in the "opts.insert" lines.

Used make_pair instead. Shorter and cleaner.

packOptions6 test - the EXPECT_NO_THROW test can be on a single line.

Fixed. On a side note, I think EXPECT_NO_THROW macro is kind of pointless. If there is exception thrown during test, the test will fail anyway. The only benefit of using it is that the (failed) test will continue execution.

No need for spaces after the opening parenthesis and before the closing parenthesis in some EXPECT_EQ checks.

Done.

src/lib/dhcp/tests/option6_addrlst_unittest.cc
No need for spaces after the opening parenthesis and before the closing parenthesis in some EXPECT_EQ checks.

src/lib/dhcp/tests/option6_ia_unittest.cc
suboptions_pack test - "sub1" declaration does not need to sbe split across two lines.

suboptions_unpack test - not sure what the cryptic comment "48 bytes" means.

Clarified.

suboptions_unpack test - in the data declaration, binary operators should have a space around them.

Done.

src/lib/dhcp/tests/option6_iaaddr_unittest.cc
basic test - in the initialization loop, binary operators should have a space around them.

Done.

src/lib/dhcp/tests/option_unittest.cc
No need for spaces after the opening parenthesis and before the closing parenthesis in some EXPECT_EQ checks.

Done.

src/lib/dhcp/tests/pkt6_unittest.cc
constructor test - No space before the "*" in the declaration of pkt1.

No need for spaces after the opening parenthesis and before the closing parenthesis in some EXPECT_EQ checks.

capture() - C++ style is to put the "*" after the return type, not before the function name.

capture() - can we line up the assignment of data elements? However, it would be clearer to statically initialize the array.

This code was actually automatically generated. Lined up assignments and added comment in iface_msg_unitest (in case we will ever use it again to convert captured packets into parsing tests).

packUnpack test - "3*Option" - should be spaces around the "*".

Done.

addGetDelOptions test - C++ style is to put the "*" after the return type, not before the function name (declaration of "parent".) Also should be no space before the closing parenthesis.

Done.

src/lib/util/buffer.h
The change appears OK, but tests/buffer_unittest.cc should be extended to check that the constructor using a vector works.

Implemented. On a side note, I'm now 100% addicted to TDD. Buffer constructor actually didn't set position properly and test found that out.

Also renamed some tests to reflect that they are for inputBuffer.

ChangeLog
Text for the entry is OK.

comment:9 Changed 8 years ago by tomek

Changes pushed and are awaiting next round of reviews.

Also included fixes for: #1282, #1295.

comment:10 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

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

  • Owner changed from stephen to tomek

The only way to cast that to struct in_pktinfo* is to use reinterpret_cast, which is only slightly better than pure C cast. I would be less readable, though. I left C style cast for now. Let me know if you prefer reinterpret_cast instead.

I would prefer reinterpret_cast, but Jinmei is opposed. We had a discussion about just this point on #1593 recently. We ended up using a separate function to perform the conversion, inside which there was a construct of the form

static_cast<desired_type*>(static_cast<void*>(pointer));

For consistency, I think we should do the casting here using a similar function.

Also, can the Linux-specific part be put into a separate function (possibly in a separate file)?

Taking as an example IfaceMgr::send(), instead of:

#if defined(OS_LINUX)
:
#endif

call something like

os_setup_interface(m, control_buf, control_buf_len_, pkt);

os_setup_interface is in a separate file and it is this file that betterholds the operating-specific stuff. (In this way for example, there would be no need to reference the Linux-specific CMSG stuff within the general functions.)

My sense of good taste (or lack of thereof) must have changed recently. I don't consider calling 4 methods (getAddress().to_v6().to_bytes().data()) to get an actual value of address from address object ugly anymore.

It's called getting old. You develop a taste for the sophisticated things in life - good food, fine wine, and the use of four methods to get an address. :-)

src/bin/dhcp4/dhcp4_srv.cc
run(): change is OK, but suggest either:

Pkt4Ptr query = IfaceMgr::instance::receive4(); // Client's message
Pkt4Ptr rsp; // Server's response

(this was done in Dhcp6Srv::run()) or

// Client's query and server's response
Pkt4Ptr query = IfaceMgr::instance::receive4();
Pkt4Ptr rsp;

(As it is, "client's message" appears to apply to both statements.)

src/bin/dhcp6/dhcp6_srv.cc
setServerID(): the comment about the DUID being generated and stored is fine. I think we should create a separate ticket to abstract the DUID into a separate class. That way we can extend the code to add other DUID types without affecting operation of the server.

setServerID(): (Comment) "all those conditions". Do you mean "All the following checks"? (Also, being pedantic, please start comments with capital letters. It is easier to read them they follow the rules of English.)

setServerID(): (Comment) "mac at least 6 bytes". Should be "MAC: at least 6 bytes". Also, it would be more descriptive to put that comment into a header with the declaration of "const size_t MIN_MAC_LEN = 6". (I note that there is already MAX_MAC_LEN symbol.)

setServerID(): with regards to checking the array for all zeroes, this would seem to be of general usefulness and could be abstracted into a separate function (perhaps in a separate "util/range_utilities.h"). To avoid the need to allocate memory for a comparison array, I would suggest something like:

template <typename Iterator>
bool
isRangeZero(Iterator begin, Iterator end) {
    return (std::find_if(begin, end,
                         std::bind1st(std::not_equal_to<int>(), 0))
            == end);
}

setServerID(): The fillling in of random numbers should be done in a separate function. It will make it easier to update. (I'm not sure than rand() has particularly good randomisation properties on all operating systems.) Also, if you are declaring a "standard", again it would be best in a separate function.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
"This test will start failing after 2030. Hopefully we will be on BIND12 by then."
Optimist!

If the isRangeZero() function above is written (and separately tested), it can be used in this test.

src/lib/dhcp/iface_mgr.cc
stubdetectIfaces(): call to "if_nametoindex" has spurious blank space inside the parentheses.

OpenSocketX(): May want to consider defining the port as "const uint16_t" (and in the header file as well). It doesn't change meaning, but it is a guarantee that "port" is not modified inside the method.

send(): as "const_cast" is unusual, a description why there is a need to write to what is considered to be an unmodifiable buffer is needed.

send(): "int result = sendmsg(..." is incorrectly indented.

receive6(): I must have missed this last time round, but:

  • Declare the objects closer to where they are used.
  • Why is "v" not initialized to zero before being used?
  • Use "static_cast<void*>()" instead of "(void*)".

src/lib/dhcp/iface_mgr.h
getIFaces() actually returns a reference to the container collection stored inside IfaceMgr. The documentation should indicate that the reference only valid for as long as the IfaceMgr object is valid.

src/lib/dhcp/libdhcp++.cc
unpackOptions6(): should be spaces around binary operators.

OptionFactoryRegister(): typo "consumed", not "consumer".

src/lib/dhcp/tests/libdhcp++_unittest.cc
packOptions4/6 tests: EXPECT_THROW should have no spaces either side of the parentheses.

src/lib/dhcp/tests/option6_ia_unittest.cc
suboptions_unpack tests: In the "ASSERT_EQ", the expected value should be the first parameter.

src/lib/dhcp/tests/option_unittest.ccBR]]
v4basic_test: there is no need to check if "opt" is defined before calling "delete".

src/lib/dhcp/tests/pkt6_unittest.cc
Thanks for lining up the initialization of data. Now for a spaces around the "=" signs?

packUnpack test: should use "static_cast<const uint8_t*>() instead of a C-style cast.

comment:12 in reply to: ↑ 11 Changed 8 years ago by tomek

Replying to stephen:

The only way to cast that to struct in_pktinfo* is to use reinterpret_cast, which is only slightly better than pure C cast. I would be less readable, though. I left C style cast for now. Let me know if you prefer reinterpret_cast instead.

I would prefer reinterpret_cast, but Jinmei is opposed. We had a discussion about just this point on #1593 recently. We ended up using a separate function to perform the conversion, inside which there was a construct of the form

static_cast<desired_type*>(static_cast<void*>(pointer));

For consistency, I think we should do the casting here using a similar function.

In my opinion that is overly bloated, but I did as you suggested. Added new header file for pktinfo conversions. There is also in_pktinfo (compared to currently used in6_pktinfo) structure in Linux and possibly other systems, so I kept name of the file only similar to actual structure (pktinfo_util.h rather than in6_pktinfo_util.h).

Also, can the Linux-specific part be put into a separate function (possibly in a separate file)?

Done. There are two outstanding ifdef(OS_LINUX) instances. One of them is about specific socket bindings (we will remove it once we start supporting BSD family) and the second
one will be remove once we have OS-specific files. (iface_mgr_bsd.cc and iface_mgr_solaris.cc).

os_setup_interface is in a separate file and it is this file that betterholds the operating-specific stuff. (In this way for example, there would be no need to reference the Linux-specific CMSG stuff within the general functions.)

CMSG and sendmsg/recvmsg is defined in POSIX (or at least it is supported everywhere). The generic mechanism allows definition of additional messages that are Linux specific.

My sense of good taste (or lack of thereof) must have changed recently. I don't consider calling 4 methods (getAddress().to_v6().to_bytes().data()) to get an actual value of address from address object ugly anymore.

It's called getting old. You develop a taste for the sophisticated things in life - good food, fine wine, and the use of four methods to get an address. :-)

You are mistaken, sir. Women are getting old. Gentlemen are just gaining experience. :-)

src/bin/dhcp4/dhcp4_srv.cc
run(): change is OK, but suggest either:
Client's query and server's response

I chose this one as it make the lines shorter.

src/bin/dhcp6/dhcp6_srv.cc
setServerID(): the comment about the DUID being generated and stored is fine. I think we should create a separate ticket to abstract the DUID into a separate class. That way we can extend the code to add other DUID types without affecting operation of the server.

Created ticket #1768. That will be two classes, actually: one for DUID itself and second for representing DUID as option.

setServerID(): (Comment) "all those conditions". Do you mean "All the following checks"? (Also, being pedantic, please start comments with capital letters. It is easier to read them they follow the rules of English.)

setServerID(): (Comment) "mac at least 6 bytes". Should be "MAC: at least 6 bytes". Also, it would be more descriptive to put that comment into a header with the declaration of "const size_t MIN_MAC_LEN = 6". (I note that there is already MAX_MAC_LEN symbol.)

Added.

setServerID(): with regards to checking the array for all zeroes, this would seem to be of general usefulness and could be abstracted into a separate function (perhaps in a separate "util/range_utilities.h").

That is true. I did as you suggested, but I think we could also start using lambda expressions. I didn't put much thought into this part of the code as it is typically done only once during whole life of a server installation (not even during every startup). Temporarily allocating couple of bytes just once is not an issue.

setServerID(): The fillling in of random numbers should be done in a separate function. It will make it easier to update. (I'm not sure than rand() has particularly good randomisation properties on all operating systems.) Also, if you are declaring a "standard", again it would be best in a separate function.

My comment about standard was more or less a joke. RFC says that DUID-EN should be followed by company defined data.

Nevertheless, having a way to generate random content may be useful. Added range_utilities.h as suggested. I also noted that headers in src/lib/util/ are named *_utilities.h, but in src/lib/util/io/ follow different pattern and use *_util.h instead. That is somewhat strange, especially that there are two new headers added in this ticket. I decided to use _utilities.h pattern.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
"This test will start failing after 2030. Hopefully we will be on BIND12 by then."
Optimist!

If the isRangeZero() function above is written (and separately tested), it can be used in this test.

Done.

src/lib/dhcp/iface_mgr.cc
stubdetectIfaces(): call to "if_nametoindex" has spurious blank space inside the parentheses.

Done.

OpenSocketX(): May want to consider defining the port as "const uint16_t" (and in the header file as well). It doesn't change meaning, but it is a guarantee that "port" is not modified inside the method.

Done.

send(): as "const_cast" is unusual, a description why there is a need to write to what is considered to be an unmodifiable buffer is needed.

Added explanation.

send(): "int result = sendmsg(..." is incorrectly indented.

Fixed.

receive6(): I must have missed this last time round, but:

  • Declare the objects closer to where they are used.
  • Why is "v" not initialized to zero before being used?
  • Use "static_cast<void*>()" instead of "(void*)".

Done.

src/lib/dhcp/iface_mgr.h
getIFaces() actually returns a reference to the container collection stored inside IfaceMgr. The documentation should indicate that the reference only valid for as long as the IfaceMgr object is valid.

Added comment.

src/lib/dhcp/libdhcp++.cc
unpackOptions6(): should be spaces around binary operators.

Added missing spaces.

OptionFactoryRegister(): typo "consumed", not "consumer".

Fixed.

src/lib/dhcp/tests/libdhcp++_unittest.cc
packOptions4/6 tests: EXPECT_THROW should have no spaces either side of the parentheses.

src/lib/dhcp/tests/option6_ia_unittest.cc
suboptions_unpack tests: In the "ASSERT_EQ", the expected value should be the first parameter.

src/lib/dhcp/tests/option_unittest.ccBR]]
v4basic_test: there is no need to check if "opt" is defined before calling "delete".

src/lib/dhcp/tests/pkt6_unittest.cc
Thanks for lining up the initialization of data. Now for a spaces around the "=" signs?

packUnpack test: should use "static_cast<const uint8_t*>() instead of a C-style cast.

comment:13 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

All fixes checked in. Tests are passing on Linux, compiling on Mac OS now.

Please review.

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

  • Owner changed from stephen to tomek

Reviewed commit 485f7966f583f54cf2694f054de6accea9c19364

I've pushed some small changes.

src/bin/dhcp4/dhcp6_srv.cc
Dhcp6Srv::setServerID() - should use the isRangeZero() function added as part of this set of changes.

src/lib/dhcp/iface_mgr.cc
Minor formatting changes have been made.

openSockets4(): Does this not leak sockets? "sock" is a local variable and appears to be discarded after receiving the value returned by openSocket(); it is not closed.

(Perhaps we should consider a "SocketPtr" object - a shared_ptr to a socket that would close the socket when the last pointer reference it was destroyed?)

openSockets6(): The logic here is very similar to openSockets4 and the common code could be abstracted into a separate method.

IfaceMgr::send() - (V4 version) a number of the local variables could be declared closer to where they are first used: in the case of "cmsg" and "result", the declaration could be combined with initialization.

Done.

Did you check that change in? "struct msghdr" and "struct iovec v" are still defined at the head of the function.

BTW, the same considerations about locality of declaration apply to the V6 version.

IfaceMgr::send() - (V4 version) although the compiler apparently accepts it for the purposes of calculating a sizeof(), it feels uncomfortable to dereference pktinfo before initializing it. Instead of "sizeof(*pktinfo)", "sizeof(in6_pktinfo)" seems safer. Or swap this statement with the one before it?

Done.

The V4 stuff has been moved to the Linux-specific code, but the V6 version of send() still has this problem.

IfaceMgr::send() - (V6 version) Use a "static_int<char*>" to cast the data for v.iov_base instead of the C-style cast.

Let me know if you prefer reinterpret_cast instead.

I do, along with a comment as to why a reinterpret_cast (as opposed to a static_cast) is necessary.

Issues remaining with IfaceMgr::receive6():

  • "result" should be defined at point of first use.
  • Should RCVBUFSIZE be a global constant? It sets the maximum receive buffer size and may be used elsewhere.
  • Given that "pkt" is being created using a constructor that just copies "result" bytes from "buf" into internal storage, is there any reason to suspect that the creation of a Pkt6 could fail?
  • ifindex should be declared close to point of first use.
  • found_pktinfo should be "bool"
  • In the "while (cmsg != NULL)" loop, it appears that the the CMSG blocks form a linked list and you are searching for the last one that matches the criteria of the "if" test. The comment implies that there is only one; if this is the case, add a "break" out of the loop when the first block is found; alternatively extend the loop termination condition to:
    while ((cmsg != NULL) && !found_pktinfo)
    
  • There appears to be no reason why "buf" should be declared static - it is not referenced outside this function.

I think we could also start using lambda expressions

True - I think we should investigate boost::lambda. However, in this case the function is descriptive of what it does and usage is clear, so isRangeZero() is useful in this case.

src/lib/dhcp/iface_mgr_linux.cc
This definitely needs documentation explaining the netlink interface.

src/lib/dhcp/tests/option_unittest.cc
v4basic_test: there is no need to check if "opt" is defined before calling "delete".

src/lib/dhcp/tests/pkt6_unittest.cc
Thanks for lining up the initialization of data. Now for a spaces around the "=" signs?

packUnpack test: should use "static_cast<const uint8_t*>() instead of a C-style cast.

I think these comments got missed.

src/lib/util/range_utilities.h
I've made minor changes to conform to the style guidelines.

src/lib/util/tests/buffer_unittest.cc
OK, but made one small change to get it to compile on Linux:

EXPECT_EQ(vec1, vec2);

was changed to

EXPECT_TRUE(vec1 == vec2);

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

Replying to stephen:

Reviewed commit 485f7966f583f54cf2694f054de6accea9c19364

I've pushed some small changes.

Th

src/bin/dhcp4/dhcp6_srv.cc
Dhcp6Srv::setServerID() - should use the isRangeZero() function added as part of this set of changes.

Fixed.

src/lib/dhcp/iface_mgr.cc
Minor formatting changes have been made.

openSockets4(): Does this not leak sockets? "sock" is a local variable and appears to be discarded after receiving the value returned by openSocket(); it is not closed.

No. Socket descriptor is returned for informational purposes only. openSockets4() calls openSocket4() that adds SocketInfo? structure to appropriate interface. See

iface.addSocket(info);

in openSocket4(). This is a safety feature. Method that creates a socket stores information about it, so there is no way to leak sockets.

Perhaps openSocket4() could be modified to return the whole structure, but the result would be the same. openSockets4() would just log that specific socket was create and would throw away the structure.

(Perhaps we should consider a "SocketPtr" object - a shared_ptr to a socket that would close the socket when the last pointer reference it was destroyed?)

openSockets6(): The logic here is very similar to openSockets4 and the common code could be abstracted into a separate method.

See above. The same rule applies to v6. Note that structures are shared. We have a uniform structure to represent v4/v6/TCP/UDP sockets. This will come in handy, when we start supporting fancier features, like failover or bulk leasequery (works over TCP).

IfaceMgr::send() - (V4 version) a number of the local variables could be declared closer to where they are first used: in the case of "cmsg" and "result", the declaration could be combined with initialization.

Done.

Did you check that change in? "struct msghdr" and "struct iovec v" are still defined at the head of the function.

Yes, I did. See commit 4fd6efcde0f9cb8d7d1513530324a389e32a978d (parent of the commit you are reviewing). cmsg and result are now in iface_mgr_linux.c. It is true that m and v could be moved closer to their use. Just did that.

BTW, the same considerations about locality of declaration apply to the V6 version.

Fixed.

IfaceMgr::send() - (V4 version) although the compiler apparently accepts it for the purposes of calculating a sizeof(), it feels uncomfortable to dereference pktinfo before initializing it. Instead of "sizeof(*pktinfo)", "sizeof(in6_pktinfo)" seems safer. Or swap this statement with the one before it?

Done.

The V4 stuff has been moved to the Linux-specific code, but the V6 version of send() still has this problem.

Fixed.

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

  • Owner changed from tomek to stephen

Replying to stephen:

Reviewed commit 485f7966f583f54cf2694f054de6accea9c19364

IfaceMgr::send() - (V6 version) Use a "static_int<char*>" to cast the data for v.iov_base instead of the C-style cast.

Let me know if you prefer reinterpret_cast instead.

I do, along with a comment as to why a reinterpret_cast (as opposed to a static_cast) is necessary.

It seems that we can get away with const_cast. That is better than reinterpret_cast.

Issues remaining with IfaceMgr::receive6():

  • "result" should be defined at point of first use.
  • Should RCVBUFSIZE be a global constant? It sets the maximum receive buffer size and may be used elsewhere.

Moved to iface_mgr.cc. Now receive4() and receive6() use the same constant.

  • Given that "pkt" is being created using a constructor that just copies "result" bytes from "buf" into internal storage, is there any reason to suspect that the creation of a Pkt6 could fail?

We could run out of memory. I also expect to have more checks implemented. Depending on how we want to approach code hardening, doing simple check that early could speed up processing (like dropping unsupported message types - seems trivial, just compare first byte).

  • ifindex should be declared close to point of first use.
  • found_pktinfo should be "bool"
  • In the "while (cmsg != NULL)" loop, it appears that the the CMSG blocks form a linked list and you are searching for the last one that matches the criteria of the "if" test. The comment implies that there is only one; if this is the case, add a "break" out of the loop when the first block is found; alternatively extend the loop termination condition to:
    while ((cmsg != NULL) && !found_pktinfo)
    

Added break statement.

  • There appears to be no reason why "buf" should be declared static - it is not referenced outside this function.

Performance reasons. We can reuse the same region of heap, rather than requiring 1500 to be reserved on stack. I admit that expected benefit of such approach is rather slim.

I think we could also start using lambda expressions

True - I think we should investigate boost::lambda. However, in this case the function is descriptive of what it does and usage is clear, so isRangeZero() is useful in this case.

I must admit that I never used lambda expressions, so it would be interesting to use them. On the other hand, using all new fancy features, just because I learned them yesterday is a very bad idea.

src/lib/dhcp/iface_mgr_linux.cc
This definitely needs documentation explaining the netlink interface.

Added links to Wikipedia article that explains netlink framing and also reference to RFC3549.
I think it is not the best use of our resources to document every piece of OS-specific interfaces.
Also, there is #1528 ticket about Linux code refactoring. Introducting too many changes in 1540 ticket will only make the merge more difficult.

src/lib/dhcp/tests/option_unittest.cc
v4basic_test: there is no need to check if "opt" is defined before calling "delete".

Removed.

src/lib/dhcp/tests/pkt6_unittest.cc
Thanks for lining up the initialization of data. Now for a spaces around the "=" signs?

Added spaces around equal signs. Also fixed other aesthetic mistakes. Equal signs are now
aligned properly and there are no more leading zeros.

packUnpack test: should use "static_cast<const uint8_t*>() instead of a C-style cast.

I think these comments got missed.

Fixed.

src/lib/util/range_utilities.h
I've made minor changes to conform to the style guidelines.

Thank you.

src/lib/util/tests/buffer_unittest.cc
OK, but made one small change to get it to compile on Linux:

EXPECT_EQ(vec1, vec2);

was changed to

EXPECT_TRUE(vec1 == vec2);

Thank you.

Code is ready for another round of reviews.

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

  • Owner changed from stephen to tomek

src/bin/dhcp6/dhcp6_srv.cc

Dhcp6Srv::setServerID() - should use the isRangeZero() function added as part of this set of changes.

Fixed.

I've removed the now-unecessary declaration of zeros from Dhcpv6Srv::setServerID() and pushed the change.

src/lib/dhcp/iface_mgr.cc

There appears to be no reason why "buf" should be declared static - it is not referenced outside this function.

Performance reasons. We can reuse the same region of heap, rather than requiring 1500 to be reserved on stack. I admit that expected benefit of such approach is rather slim.

I don't think it is even that - reserving it on the stack just means that a larger number will be substracted from the stack pointer than otherwise would be the case.

Other
Running the tests gave me:

Running test: dhcp6_test.py
[b10-dhcp6] Server failed: Failed to bind socket 4 to fe80::221:91ff:fe7e:55de/port=547
/usr/lib/python3.2/unittest/case.py:370: ResourceWarning: unclosed file <_io.FileIO name=7 mode='wb'>
  function()

... but the system did not report an error.

(The IP address printed is the one associated with my Wireless LAN interface)

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

  • Owner changed from tomek to stephen

Replying to stephen:

src/bin/dhcp6/dhcp6_srv.cc

Dhcp6Srv::setServerID() - should use the isRangeZero() function added as part of this set of changes.

Fixed.

I've removed the now-unecessary declaration of zeros from Dhcpv6Srv::setServerID() and pushed the change.

Thanks.

src/lib/dhcp/iface_mgr.cc

There appears to be no reason why "buf" should be declared static - it is not referenced outside this function.

Performance reasons. We can reuse the same region of heap, rather than requiring 1500 to be reserved on stack. I admit that expected benefit of such approach is rather slim.

I don't think it is even that - reserving it on the stack just means that a larger number will be substracted from the stack pointer than otherwise would be the case.

Ok, removed static.

Other
Running the tests gave me:

Running test: dhcp6_test.py
[b10-dhcp6] Server failed: Failed to bind socket 4 to fe80::221:91ff:fe7e:55de/port=547
/usr/lib/python3.2/unittest/case.py:370: ResourceWarning: unclosed file <_io.FileIO name=7 mode='wb'>
  function()

... but the system did not report an error.

(The IP address printed is the one associated with my Wireless LAN interface)

The purpose of this test is to see if b10-dhcp6 process could be started, not if it can bind sockets. Please re-run tests from root if you are concerned about socket binding. There is a separate ticket #1503 about adding capability to listen on non-root port.

Extended test code to print out a note about that.

Changes pushed to trac1540 branch.

I believe this addresses all raised comments. In my opinion the code is ready for merge, unless there are new discussion points.

comment:19 Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit be560f4516ae9b0bbbad1740a519feed99c04170.

No new discussion points - please merge.

Stephen

comment:20 Changed 8 years ago by tomek

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

Code finally merged to master. This was a long one.

Note: See TracTickets for help on using tickets.