Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#1485 closed defect (fixed)

IOAddress::getAddress() breaks the concept of the asiolink

Reported by: jinmei Owned by: tomek
Priority: medium Milestone: Kea0.8
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: 1
Total Hours: 5 Internal?: no

Description

I just happen to notice a (relatively) new method "getAddressas"
is added to the asiolink::IOAddress class.

This breaks the basic concept of asiolink: The fundamental idea of
this wrapper module is to hide asio-specific details from other part
of BIND 10. Exposing asio's internal definition this way will make it
useless.

Please reconsider the interface: if the current IOAddress class is
insufficient in terms of functionality, we need to add a higher level
method to provide that necessary functionality, rather than exposing
asio::ip::address and have the caller use the underlying ASIO
interface.

According to git blame and how this method is used, I guess it was
added for DHCP, so I'm specifying "DHCP" as the sub-project.

Subtickets

Attachments (1)

0001-1485-Remove-all-traces-of-ASIO-includes-from-public-.patch (12.9 KB) - added by muks 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 8 years ago by shane

  • Milestone changed from New Tasks to DHCP 2011

comment:2 Changed 6 years ago by muks

  • Milestone changed from DHCP Outstanding Tasks to DHCP-Kea1.0-proposed
  • Priority changed from medium to high

For the DHCP release, this should be fixed at a high priority to avoid internal use of asio:: leaking into our public API.

comment:3 Changed 6 years ago by muks

See also: #3134

comment:4 Changed 6 years ago by tomek

  • Milestone changed from DHCP-Kea1.0-proposed to DHCP-Kea1.0-alpha

Moving to DHCP-Kea1.0-alpha as agreed on Kea status meeting (2014-01-08)

comment:5 Changed 6 years ago by tomek

  • Priority changed from high to medium

Decreasing priority as agreed on Kea status meeting (2014-01-08)

comment:6 Changed 6 years ago by tomek

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

comment:7 Changed 6 years ago by tomek

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

The code is ready for review. Since this is an internal change, I think that ChangeLog? entry is not necessary.

comment:8 Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 0 to 3
  • Total Hours changed from 0 to 3

comment:9 Changed 6 years ago by muks

Hi Tomek

I saw your message on Jabber this morning and looked at the trac1485 branch.

You probably already thought that I would ask for the ASIO includes to be removed from the public branch. :) This can be achieved by using a pimpl idiom. I've attached a commit to this ticket which does this. However this shows up other issues in the DNS code:

  • Although dns_server.h is not disted (and so will not produce ABI issues later), it directly uses ASIO functions/constants. From the documentation in asiolink.h, it is not very clear who is supposed to include the ASIO headers in this case. Is it the program code? Some of the headers say this, but I'm not sure if this is the intended approach. This needs to be resolved, and resolving this is a bigger task than this ticket.
  • Similarly for other bits of the code.
  • The IOAddress(const asio::ip::address&) constructor was removed in the patch, and it seems some parts of the DHCP code uses it. If the patch is adopted, this code needs to be updated. There is a sub-optimal workaround for this in the patch itself, but it can be fixed properly if it is going to be used in a hotspot.

The changes to the asiolink library look fine. You may want a DHCP developer to review the DHCP code changes.

comment:10 Changed 6 years ago by stephen

  • Owner changed from muks to tomek

comment:11 Changed 6 years ago by tomek

  • Owner changed from tomek to UnAssigned

Good DHCP folks! Please review the changes on branch. Please DO NOT review the attached patch (It will be moved to a separate ticket).

comment:12 Changed 6 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:13 Changed 6 years ago by tmark

  • Add Hours to Ticket changed from 3 to 1
  • Owner changed from tmark to tomek
  • Total Hours changed from 3 to 4

src/lib/asiolink/tests/io_address_unittest.cc

+// test v6-specific operations
+TEST(IOAddressTest, v6specific) {

It's a bit of misnomer to label the new test v6specific. It verifies
that isV4() gives the correct results as well. Really it verifies "address classification methods"


perfdhcp/test_control.cc -

line 1642 you changed the test test that the address is v6 rather is
NOT v4:

-    if (!yiaddr.getAddress().is_v4()) {
+    if (yiaddr.isV6()) {
         isc_throw(BadValue, "the YIADDR returned in OFFER packet is not "
                   " IPv4 address");

yet a similar check on line 1753, you left as a NOT v4:

-    if (!yiaddr.getAddress().is_v4()) {
+    if (!yiaddr.isV4()) {
         isc_throw(BadValue, "the YIADDR returned in OFFER packet is not "
                   " IPv4 address");

I would recommend you change the test at line 1642 to is NOT v4. First, they should be the same, and secondly, when v16 comes out it won't right anymore ;).


I do not need to see this ticket again.

comment:14 Changed 6 years ago by tomek

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

Thanks for your review, guys. Code merged, ticket closed.

comment:15 Changed 6 years ago by tomek

  • Total Hours changed from 4 to 5
Note: See TracTickets for help on using tickets.