#5207 closed enhancement (complete)

Refactor HostReservationParser to return the reservation rather than add it to the CfgMgr.

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Kea1.2-final
Component: host-reservations Version: git
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: 5
Total Hours: 5 Internal?: no

Description (last modified by marcin)

When we implement the code for adding host reservations over the command channel, it will be useful to reuse the HostReservationParser. However, this class currently uses non-virtual addHost function to store reservations in the CfgMgr. Rather than calling addHost we'd rather want the parser to return parsed information. That way we can easily reuse the parser.


Change History (7)

comment:1 Changed 19 months ago by marcin

  • Type changed from defect to enhancement

comment:2 Changed 19 months ago by marcin

  • Description modified (diff)
  • Summary changed from HostReservationParser::addHost must be virtual for adding host reservations over command channel to Refactor HostReservationParser to return the reservation rather than add it to the CfgMgr.

comment:3 Changed 19 months ago by marcin

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

comment:4 Changed 19 months ago by marcin

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

The HostReservationParser and the HostReservationListParser have been modified to return parsed values rather than store them in the CfgMgr. This solves the problem we are addressing, i.e. make HostReservationParser independent from CfgMgr, however, it moves the dependency to the subnet parsers. We should address this at some point but I don't want to spend time on this now. What I have done here is good enough for what we're trying to achieve with parsing host reservations received over the command channel.

No ChangeLog is needed as it is not user visible change.

comment:5 Changed 19 months ago by tomek

  • Milestone changed from Kea-proposed to Kea1.2-final
  • Owner changed from UnAssigned to tomek

As discussed on a meeting, this is requires for kittiwake.

comment:6 Changed 19 months ago by tomek

  • Add Hours to Ticket changed from 0 to 5
  • Owner changed from tomek to marcin
  • Total Hours changed from 0 to 5

I have reviewed your changes. The code is mostly good, but there's one thing that requires explanation. Why did the new code you wrote expected to throw isc::exception, rather than specific type? I dug around and found an interesting bug underneath.

This happened in HostReservationsListParserTest?.duplicatedIdentifierValue4. Each loop iteration attempts to insert two hosts, both with the same identifier. It appears each iteration returned a different exception type. First returned DuplicateHost?, while second and all follow up iterations returned ReservedAddress?. That's because in the first iteration the first host insertion succeeded and the second failed, because of duplicated identifier. All following iterations failed, because the address was already reserved. I added clear() call, but that uncovered other problems. Now the code stopped throwing for some identifiers. This was solved by adding extra checks in CfgHosts::add4() method.

It actually took a while because I figured out how this stuff is working. When you look at CfgHosts::add(), it unconditionally calls add4 and add6. The names are misleading and so are their descriptions. add4 does not add just IPv4 hosts, it adds both v4 and v6 hosts, so its better name would be addGeneric or addCommon. add6 adds IPv6-specific elements, so better name for it would be addv6Specific or add6details. Descriptions simply stated "add4 adds IPv4 host to the reservation list", the same for v6. Fixed this as well.

Please pull and review.

If you agree with my changes, the ChangeLog? should mention that additional sanity checks were implemented when adding host reservations. If you're ok with them, this ticket is ready to go.

comment:7 Changed 19 months ago by marcin

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

Merged with commit ea42c6f479918235ae4a67a60d08720e2664720c

Note: See TracTickets for help on using tickets.