Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#878 closed enhancement (fixed)

Code to bind to address/port

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

Description (last modified by tomek)

Provide functionality of a DHCPv6 echo server:

  • detect interfaces
  • create and bind sockets to available address
  • receive packets from DHCPv6 clients
  • echo back packets to clients

Those changes are fairly large. Currently dhcp6 code (with tests) is 1,6KLOC.

Subtickets

Change History (26)

comment:1 Changed 9 years ago by shane

  • Sub-Project changed from DNS to DHCP

comment:2 Changed 9 years ago by shane

  • Milestone changed from New Tasks to Sprint-DHCP-20110517

comment:3 Changed 8 years ago by shane

  • Milestone changed from Sprint-DHCP-20110628 to Sprint-DHCP-20110712

Milestone Sprint-DHCP-20110628 deleted

comment:4 Changed 8 years ago by tomek

Changes to this ticket grew up a bit. Besides simple binding address/port, it also provides DHCPv6 echo server that sends received messages back to clients. To achieve this, changes also cover:

  • Pkt6 class - for handling packets
  • Addr6 class - for address conversion and storage
  • IfaceMgr? class - a wrapper around socket creation, sending, receiving data, interface detection (not functional yet, stubbed for now)
  • Dhcpv6Srv class - this will become full featured DHCPv6 server, but it just echoes the data back to clients.

There are 12 tests implemented using gtest. This is the first time I used gtest, and I haven't had time to read the whole tutorial properly, so there may be odd things in there.

The code is less commented that I would like it to be. I'll document it once I get back from vacation.

As there is no interface detection implemented, stub for that is developed. Instead of reading information about available interfaces, it just reads first line from interfaces.txt and binds to specified address and interface. This temporary functionality will go away as soon as interface detection is implemented.

Please review.

comment:5 Changed 8 years ago by tomek

  • Description modified (diff)
  • Owner set to UnAssigned
  • Status changed from new to reviewing

comment:6 Changed 8 years ago by tomek

  • Description modified (diff)

comment:7 Changed 8 years ago by jinmei

From a quick look, it doesn't follow the coding style guideline very much.
e.g. missing parentheses around 'return', redundant (in terms of the
guideline) space between type and '*' (as in 'int * foo').

To avoid discussing the obvious in the review cycle, I'd suggest
(re)reading the guideline at http://bind10.isc.org/wiki/CodingGuidelines
and fixing them before continuing.

Some other things I happened to notice:

  • in general, I'd avoid using C-style casts
  • there are some points that perform 'new' and use the result in its bare form. I've not fully followed the code logic, but in general it seems dangerous to me. That style of code is often exception unsafe.
  • I suggest gtest code within an unnamed namespace.
  • variables/methods of gtest classes should better be 'protected', than 'public'
  • cppcheck complained about a couple of points:
    src/bin/dhcp6/iface_mgr.cc:372: check_fail: The scope of the variable found_pktinfo can be reduced (information,variableScope)
    src/bin/dhcp6/iface_mgr.cc:52: check_fail: Function parameter 'name' should be passed by reference. (performance,passedByValue)
    

(p.s. this is just a quick comments I happen to notice while peeking into
the code out of curiosity. I'm not committed to a full review (yet) :-)

comment:8 Changed 8 years ago by tomek

Changes suggested by Jinmei are done. The code now better adheres to coding standards, code is (hopefully) exception safe, cppcheck warnings removed, gtest code is now in unnamed namespace.

gtest functions stay public. They are public for a reason - they are used in test fixtures to poke with internal workings of tests classes.

I'm aware of serious limitations of the code:

  • interface detection is not implemented. This makes automated/portable testing very difficult. That's why the code is less tested than I would like it to be. Once interface detection is implemented, more thorough tests will be implemented.
  • code uses cout stream, rather than logging
  • we should probably define isc::dhcp or isc:dhcp6 namespace, rather than use generic isc
  • Pkt6 class requires serious development. For now it is just a generic packet container.

comment:9 Changed 8 years ago by tomek

  • Owner changed from UnAssigned to stephen

comment:10 Changed 8 years ago by tomek

  • Owner changed from stephen to shane

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

Build failed on my system due to a number of variables that were set but never used. I removed these, and also updated the documentation based on the now-missing options. (This box is using gcc 4.6)

I've pushed these, since they are basically trivial changes. Review continues!

comment:12 Changed 8 years ago by shane

Okay, looking at the Addr6 bit:

  • I note that the asio library has some IPv6 functions in its asio library, and we use that library:

http://www.boost.org/doc/libs/1_47_0/doc/html/boost_asio/reference/ip__address_v6.html

It may not be what we need, but in the future make sure to have a look for existing libraries.

  • I don't like the overloaded constructor from a character constant. It makes it non-obvious from reading the code what is actually going on. I'd suggest adding a non-member factory function to create Addr6 instances from printable strings, ParseAddr6() or something like that.
  • We don't use hard tabs in BIND 10. s/I/ /g should fix it. :)
  • linkLocal() and multicast() should contain a comment or even better a reference to the RFCs where they are defined.
  • Why don't you use INET6_ADDRSTRLEN which is guaranteed to be long enough for an IPv6 string, rather than defining MAX_ADDRESS_STRING_LEN on our own?

comment:13 Changed 8 years ago by shane

I know Pkt6 is just a bare skeleton, but...

  • I think rather than changing ownership of the allocated packet, we should use a smart pointer instead, or more properly the shared_array type from Boost. It is designed for just this kind of access pattern, and means we don't have to be as careful about memory.


http://www.boost.org/doc/libs/1_46_1/libs/smart_ptr/smart_ptr.htm

  • If you really want to stick with the current allocation style, then I think it is better to let the out of memory exception exit the Pkt6::Pkt6(int datalen) constructor rather than catch it.
  • While all member fields should be commented, the iface and ifindex especially need to be explained (I know what they mean but they probably won't be obvious to anyone who has not worked with interfaces directly).

comment:14 Changed 8 years ago by shane

Moving on to the iface_mgr class:

  • We probably don't need to expose the instanceCreate() method, since this is invoked by the instance() method. Also, Boost provides a utility function to only call a function once, so we can use that rather than checking for duplicate invocations and throwing an exception in instanceCreate().

http://www.boost.org/doc/libs/1_32_0/doc/html/call_once.html

  • The openSockets() method can be private, as can openSocket() and joinMcast() actually.
  • You may as well pass a stream to printIfaces, so printIfaces(ostream& out = cout) or something like that.
  • Should the getIface() methods return constant values? I'm thinking:
        Iface const* getIface(int ifindex) const;
        Iface const* getIface(const std::string& ifname) const;
  • I generally prefer returning NULL instead of (0) (as in getIface()), but I don't know if that is actually a BIND 10 style or just my personal preference. ;)
  • Do we actually need a constructor for Iface() objects that don't have a name and index defined? Isn't such an object actually undefined in a way? (I can see we might want a way to define an instance with a name only, so that if such an interface becomes available we can use it, but we may want to use a special interface ID or set a flag for that case... or perhaps make that a separate class.)
  • As you noted, we have to convert this to the logging framework. Stephen can probably help with that when he's back from vacation.
  • The IfaceMgr() constructor is not exception-safe. The control_buf_ allocation can be moved to the end of the function, since it is not used in the constructor itself. The detectIface() helper function should also be careful to de-allocate all of its memory when it encounters an error (possibly not so important right now since it is a throw-away technique, but a comment should be added so it is not forgotten). Likewise, the openSockets() function should close any open sockets if it fails. (For example the 1st openSocket() call succeeding and the 2nd failing.)
  • openSocket() should close the file descriptor when returning an error, such as with setsockopt() or bind().
  • It needs to be clearly documented that the to/from information (addr, port, interface) is all included in the packets used in send() and receive().
  • In receive(), is 1500 a safe value? I know that jumbo packets exist, and in any case a UDP packet can be more than 1500 bytes right?
  • I tend to think receive() on an unknown interface should be considered an error. We can't reply to it, correct?
  • Is it possible we will bind multiple ports on a single address? Maybe receive() should track that too...

comment:15 Changed 8 years ago by shane

The dhcp6_srv looks okay. (I considered briefly whether we should look at using ASIO for this too, but since we will only ever be a simple UDP recv/send server, that doesn't really make much sense.)

comment:16 Changed 8 years ago by shane

Looking over the tests...

I had one of the tests fail:

[ RUN      ] IfaceMgrTest.getIface
Interface checks. Please ignore socket binding errors.
IfaceMgr initialization.
Interface detection is not implemented yet. Reading interfaces.txt file instead.
Please use format: interface-name link-local-address
Failed to read interfaces.txt file.
Interface detection failed.
IfaceMgr creation failed:std::exception
[       OK ] IfaceMgrTest.getIface (0 ms)
[ RUN      ] IfaceMgrTest.detectIfaces
iface_mgr_unittest.cc:117: Failure
Value of: ifacemgr.getIface("eth0") != NULL
  Actual: false
Expected: true
[  FAILED  ] IfaceMgrTest.detectIfaces (0 ms)

It is not repeatable, sadly. I'm not sure what could have caused this, as the test looks pretty straightforward. :(

comment:17 Changed 8 years ago by shane

Digging into the Addr6Test:

  • The constructor test doesn't have a bad string for "plain". This actually points out to me that the constructor itself needs to handle that case (or even better, have that handled by the factory method I suggested above).
  • You should also go ahead and add a test for the constructor without arguments, and with a socket structure. Code coverage measurements will be happy. :)

comment:18 Changed 8 years ago by shane

And on to the IfaceMgrTest?:

  • If you decide to keep a constructor for "empty" Iface structures, then you need to test that too.
  • Test the getPlainMac() call too.
  • openSocket() should check for error conditions too, for example by trying to bind to the same port twice.
  • openSockets() can be tested in a manner similar to how detectIfaces() is, by providing a hand-crafted text file.
  • It would be nice if joinMcast() could be tested... but we actually need a multicast interface for that. When we start using real interface detection, we can run such a test if any multicast interface exists. For now it's fine to omit it I guess.

comment:19 Changed 8 years ago by shane

Final test, dhcpv6_srv_unittest.cc... When we add support for config session then the dhcpv6srv test will be more interesting. :)

comment:20 Changed 8 years ago by shane

  • Owner changed from shane to tomek

Okay, I think this is ready for some work by Tomasz now. The basic ideas are good, and there are no obvious coding bugs. Looking forward to a 2nd review!

comment:21 in reply to: ↑ 11 Changed 8 years ago by shane

Replying to shane:

Build failed on my system due to a number of variables that were set but never used. I removed these, and also updated the documentation based on the now-missing options. (This box is using gcc 4.6)

I've pushed these, since they are basically trivial changes. Review continues!

This bug was previously reported in #1152. When we merge this branch we should also mark that one as resolved.

comment:22 Changed 8 years ago by tomek

Thanks for thorough review. I did fix most of what was pointed out. In cases where I chose to keep the code as it is, explanation is provided below.

Addr6:

  • Class replaced completely with asiolink::IOAddress. Class and tests removed. This class was memory optimal (sizeof(Addr6) == 16), similar design for Addr4 would be 4 bytes long. That is not the case with IOAddress (sizeof(IOAddress)==32). It is good that Addr6 is kept in GIT history. We may decide to use it someday.

Pkt6:

  • now uses boost::shared_array to manage its buffer.

IfaceMgr?:

  • check_once was NOT used. It includes phtreads and is expected to be used for thread synchronization. That would add extra dependencies with potential problems, and benefit would be minimal. Made IfaceMgr::createInstance() private instead.
  • openSockets() and isMulticast() is now protected (it is used in tests, has to be accesible from derived class, so it can't be private).
  • printIface() now takes ostream as parameter (with cout being default value)
  • getIface() keeps returning non const pointers. There's a good reason to do that. There may be some modifications done to returned interface. For example statistics may be changed. Simple statistics can be updated within IfaceMgr? itself, but in time we will get also the fancy ones, e.g. how many clients on this interface failed authentication checks. If you insist on having that const, I won't object and will add it, but it will be worse from design perspective (methods belonging to Iface will be implemented in IfaceMgr?).
  • NULLs are now used instead of 0
  • Iface() constructor is removed
  • openSockets() now closes open socket when error is detected.
  • Many functions are now described. Doxygen format is used.
  • Buffer size increased to 64k (not necessary, as there are no client messages that big; RFC3315 mentions only server packet fragmentation; also size was already checked during reception, we are safe in case of strange things client may happen to throw at us)
  • packets received over unknown interface are no longer processed
  • Binding multiple addresses on the same port would only be useful in unicast communication (see unicast option in DHCPv6). This will be implemented after interface detection is implemented.
  • Changed XXX to TODO

Addr6Test:

  • Addr6 class removed and so is its test.

IfaceMgrTest?:

  • fixed problem with getIface("eth0") != NULL (IfaceMgr? is a singleton, so instance from previous test was used)
  • added test for trying to bind the same address/port twice
  • added comment that getPlainMac() has to be tested as soon as interface dectection is implemented. Currently MAC is not detected and it doesn't make much sense to implement fake MAC detection, just to write test that reports back fake MAC.
  • added test for multicast. It will be expanded once we get interface detection routines and will be able to to find non-multicast interface.

All changes pushed to trac878.

comment:23 Changed 8 years ago by tomek

  • Owner changed from tomek to shane

comment:24 Changed 8 years ago by shane

  • Owner changed from shane to tomek

Okay, it looks good, sorry for the delay.

Only two things of note on this last round.

First, the IfaceMgr? constructor is still not exception-safe. It can still leak memory, and possibly cause other issues. This isn't too much of a problem because we don't plan on continuing if we have an exception there, and because we need to improve it to deal with real interfaces, but I thought I'd mention it.

Second, Google Test choked on me because one of the tests wanted to output a message on error:

    EXPECT_EQ(sendPkt.remote_addr_, rcvPkt->remote_addr_);

I've added an operator to convert addresses to C-style strings in the asiolink library and pushed it to this branch. It fixes the error, and hopefully is straightforward enough not to be scary.

If this looks good to you, then please go ahead and merge. (Sorry again about the delay.)

comment:25 follow-up: Changed 8 years ago by tomek

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

Checked that make, make check and make distcheck all pass. Branch 878 merged to master.

comment:26 in reply to: ↑ 25 Changed 8 years ago by jinmei

Replying to tomek:

Checked that make, make check and make distcheck all pass. Branch 878 merged to master.

Don't forget deleting the branch when you are down with it.
See http://bind10.isc.org/wiki/GitWorkflow "Deleting our branch".

Note: See TracTickets for help on using tickets.