Opened 8 years ago

Closed 6 years ago

#2246 closed task (complete)

Implement cross-OS interface detection routines

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Kea0.8
Component: libdhcp 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

It seems that all OSes we care about (Linux, BSD, Mac OS, Solaris) support getifaddrs. We should implement interface detection that uses that function.

Subtickets

Attachments (1)

patch.2246 (8.5 KB) - added by dclink 6 years ago.
Both BSD interface lookup code and unit tests

Download all attachments as: .zip

Change History (24)

comment:1 Changed 7 years ago by dclink

  • Owner set to dclink
  • Status changed from new to assigned

comment:2 Changed 7 years ago by dclink

Ready for first review. Please let me know what I have to correct (as I can all understand...) and to discuss technical choices for unit tests as well.

comment:3 Changed 7 years ago by dclink

  • Owner changed from dclink to tomek
  • Status changed from assigned to reviewing

comment:4 Changed 7 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20130904
  • Owner changed from tomek to UnAssigned

comment:5 Changed 7 years ago by tomek

  • Milestone changed from Sprint-DHCP-20130904 to Sprint-DHCP-20130918

comment:6 Changed 6 years ago by tomek

  • Milestone changed from Sprint-DHCP-20130918 to Sprint-DHCP-20131016

comment:7 Changed 6 years ago by dclink

Added sun version, pretty similar to BSD. Solaris based systems has unsigned long for ifa_flags (ifaddrs.h) so for this one, Iface::flags_ matches it.

comment:8 Changed 6 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:9 Changed 6 years ago by tomek

I'm reviewing the code now.

comment:10 Changed 6 years ago by tomek

  • Owner changed from tomek to dclink

Thanks a lot for the patch. Sorry it took so long.

It looks good in general, but there are couple things that have to be fixed before we can merge it.

Documentation has to be updated. Please take a look at doc/guide/bind10-guide.html, section 19.1.

Developer's guide (cd doc; make devel; open doc/html/index.html) needs minor update, too. See src/lib/dhcp/libdhcp++.dox.

I've cleaned up your patch a bit and checked it into trac2246. I've submitted it to run on our test farm (http://git.bind10.isc.org/~tester/builder/builder.html or http://git.bind10.isc.org/~tester/builder/builder-new.html). Please check out if there are any build failures reported on trac2246.

Both BSD and Solaris versions detect all interfaces, but report only those with at least one address. They should report all interfaces, regardless if there are addresses or not. That is useful for cases when interface is not fully configured (down; not associated wifis, etc)

iface_mgr_bsd.cc and iface_msg_sun.cc
Please try to use more meaningful names. Instead of itf, use iface.

iface_mgr_unittest.cc
The line

#if defined(OS_LINUX) || defined(OS_BSD) || defined(OS_SUN)

does not make sense. We don't run on any other OSes, besides Linux, BSD or Solaris.

Functions should have Doxygen comments. They should include parameter names. We typically keep function prototypes in .h files, but your decision to keep checkIfIndex, checkIfFlags and checkIfAddrs in .cc file was correct. This way we don't need to include headers to have struct ifaddrs in a header.

The code that checks MAC address needs improvement:

        int i = 0;
        while(i < 6) {
            if(* (p + i) != * (iface.getMac() + i)) {
                return (false);
            }
            ++ i;
        }

I've added a @todo that explains the issue: MAC address is 6 bytes long for IEEE802 family interfaces only (Ethernet, Wifi, Bluetooth, etc.). For some interfaces, e.g. IPv6-over-IPv4 tunnels, it is only 4 bytes long. For other, more exotic ones, like infiniband, it is 20 bytes long. That length has to be checked. And the loop cited above can be done with a single call to memcmp().

We try to keep the number of printfs and couts in unit-tests to minimum. Please consider removing some of the messages printed. This is only a suggestion. If you think they should stay, I won't object.


Please propose ChangeLog entry.

Ok, the ticket is back with you, dclink. Please address the issues mentioned and provide a patch against trac2246 branch. Once you do that, reassign the ticket back to me.

Changed 6 years ago by dclink

Both BSD interface lookup code and unit tests

comment:11 Changed 6 years ago by dclink

  • Owner changed from dclink to tomek

Made some changes except I brought back ones from iface_mgr.h, I think it is better to respect the natural size of the underlying ifreq field ...

comment:12 Changed 6 years ago by tomek

  • Milestone changed from Sprint-DHCP-20131016 to Sprint-DHCP-20131204

comment:13 Changed 6 years ago by tomek

  • Owner changed from tomek to UnAssigned

Applied & cleaned up dclink's patch.

Also proposed ChangeLog? entry.

Submitted test run on build farm.

Can someone review my changes in ac0abc3cf7c512c87122e0f5ead4f0c2fca04890?

comment:14 Changed 6 years ago by tomek

Oh, I checked that the tests pass on Linux and FreeBSD.

comment:15 Changed 6 years ago by tomek

I scheduled test run on trac2246. It seems that there's an issue on OpenBSD:
http://git.bind10.isc.org/~tester/builder//BIND10/20131210221902-OpenBSD5-amd64/logs/unittests.out

comment:16 Changed 6 years ago by tomek

I just installed OpenBSD 5.4 and the issue does not reproduce :(

comment:18 Changed 6 years ago by tomek

The issue does not reproduce on latest OpenBSD5.4. Marcin suggested that the reason may be CMSG issue on OpenBSD that was recently fixed (workarounded really, as it is OpenBSD kernel bug) in #1824. So I merged latest master to trac2246 branch. This didn't help :( (see http://git.bind10.isc.org/~tester/builder/BIND10/20131213104106-OpenBSD5-amd64/logs/unittests.out).

comment:19 Changed 6 years ago by tomek

I rerun the tests on OpenBSD with properly merged fix for #1824. The tests are now passing (http://git.bind10.isc.org/~tester/builder/BIND10/20131213164118-OpenBSD5-amd64/logs/unittests.out). I did push Solaris11 fix and that is passing as well.

comment:20 Changed 6 years ago by tomek

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

Ok, the tests pass on OpenBSD, I managed to fix the tests on Solaris 11, so the code is now finally merged to master!

It is likely that there will be a small follow up. As far as I can tell, there's no getifaddrs () interface on solaris 10, so the code will likely fail to build on solaris 10. We don't want to support sol10, so we'll probably need to implement some #ifdef that will just skip building there. But that can be done in a separate ticket. This one should be closed.

Thanks for your patch, dclink!

comment:21 Changed 6 years ago by tomek

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:22 Changed 6 years ago by tomek

  • Owner changed from UnAssigned to tomek
  • Status changed from reopened to assigned

comment:23 Changed 6 years ago by tomek

  • Resolution set to complete
  • Status changed from assigned to closed

Sorry for the noise. I just needed to reassing it to myself before closing.

Note: See TracTickets for help on using tickets.