Opened 4 years ago

Closed 4 years ago

#4108 closed enhancement (complete)

Reorganize DHCPv4 code flow (split run() into run() + processPacket())

Reported by: tomek Owned by: fdupont
Priority: medium Milestone: Kea1.1
Component: dhcp4o6 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: 0
Total Hours: 4 Internal?: no

Description (last modified by fdupont)

During the discussion about Dhcp4o6Design, it became apparent that the DHCPv4 code flow needs adjustment in how the run() method is implemented. The run() command should be split into run() proper that calls processPacket().

This new processPacket() method will be called from run(), but also may be called from other places, e.g. a callback that receives 4o6 packet.

Subtickets

Change History (12)

comment:1 Changed 4 years ago by tomek

  • Component changed from Unclassified to dhcp4o6

comment:2 Changed 4 years ago by tomek

Couple more details: the main DHCPv4 server loop is in src/bin/dhcp4/dhcp4_srv.cc file, see Dhcp4Srv::run() method. This ticket should add a new Dhcp4Srv::processPacket(const Pkt4Ptr& pkt).

comment:3 Changed 4 years ago by jinmei

It's ready for review on the trac4108 branch (of github).

The most important change is 0a107ba0eaf00f6bcf2146392b2e68c3069317a1. I separated it from the next commit (which is only for editorial fixup) so the major change is easier to understand.

eade7b958d9ad19943be84c8f42513995bcf5f71 is an unrelated fix that I happened to notice. I believe this is correct, but if this is controversial we can revert it.

comment:4 Changed 4 years ago by tomek

  • Milestone changed from Hackathon-ietf to Kea1.1

Moving to 1.1 milestone, as discussed on 2016-01-07 Kea meeting.

comment:5 Changed 4 years ago by fdupont

  • Description modified (diff)

Note the branch exists only in the github repo.

comment:6 Changed 4 years ago by fdupont

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

I imported the branch into the Kea repo. I'll check if it is ready for review.

comment:7 Changed 4 years ago by fdupont

Read the code: there were two related changes since it was written:

  • the sent hook has the query as argument (updated)
  • there is a global try to catch unexpected exception: IMHO we should introduce a new method named run_one() and call it as the body of the first try (not yet done: require comments about this idea).

A final note: in the DHCPv4-over-DHCPv6 context the final sendPacket() outside processPacket().

comment:8 Changed 4 years ago by fdupont

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

Addressed the second point (try stuff) in the #4266 ticket so now #4108 is ready for review.

comment:9 Changed 4 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:10 Changed 4 years ago by tomek

  • Owner changed from tomek to fdupont

I reviewed the changes on origin/trac4108 (internal repo) and have several comments:

dhcp4_srv.h
Comment for run(): s/processPakcet/processPacket/

dhcp4_srv.cc
Comment in processPacket():
s/increase type specific packets/increase type specific statistic/

I also updated AUTHORS file to reflect Jinmei's contribution.

This branch was based on an older master and there were couple other changes on master. It would be useful to do git rebase before merging and then re-review the changes. Actually, I did rebase trac4108 and pushed as trac4108_rebase.

Please verify that it is ok. If it is, please apply those two changes above and merge.

comment:11 Changed 4 years ago by tomek

  • Total Hours changed from 0 to 4

Updated hours spent on review. The review itself was short, but I spent some time on the rebase.

comment:12 Changed 4 years ago by fdupont

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

Merged with applied comments and #4266 to trac4267 (trac4267_base tag). Closing.

Note: See TracTickets for help on using tickets.