Opened 7 years ago

Closed 7 years ago

#3145 closed enhancement (complete)

PD: Add support for IA_PD and IAPREFIX options

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20130918
Component: dhcp6 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

DHCPv6 support for Prefix Delegation requires those two options.

Subtickets

Change History (9)

comment:1 Changed 7 years ago by tomek

  • Status changed from new to assigned

comment:2 Changed 7 years ago by tomek

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

comment:3 Changed 7 years ago by tomek

The code is ready for review. There's a new class for IAPREFIX. Existing class has been adapted to handle IA_PD.

comment:4 Changed 7 years ago by tomek

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

comment:5 Changed 7 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:6 follow-up: Changed 7 years ago by tmark

  • Owner changed from tmark to tomek

General question:

We have a class IOAddress (for better or worse), are we or should we have a class for IPv6Prefix? This is perhaps a larger picture question than this ticket's scope.


option6_iaprefix.h

"The major differences are fields order and prefix has also additional prefix length field."

This doesn't quite make sense. Could you rephrase?


option6_iaprefix.h

I have been told that Doxygen comments should use the word Constructor, not "Ctor" or "ctor".


option6_iaprefix.h

"It make everyone's like much easier,"

I think you meant:

"It makes everyone's life much easier"


option6_iaprefix.h

line spacing is "non-standard":

    /// @return string with text representation.
    virtual std::string
    toText(int indent = 0);

option6_iaprefix.h

pack and unpack methods can both throw but it is not mentioned in the header commentary.
In the case of the latter, a throw in unpack renders the contents of the object invalid,
the user should be made aware of this.


option6_iaprefix_unittests.cc

The unit testing only tests the "sunny path". It does not test invalid address in the packet,
which should case pack() to throw, or an invalid distance which should cause unpack() to throw.

Also, the constructor invocation that is there, should probably in an ASSERT_NO_THROW() because
if it throws the test will blow up.


option6_ia_unittests.cc

line 79 and 211, constructors should be within an ASSERT_NO_THROW(), yes?
line 85, pack() should also be in an ASSERT_NO_THROW?


Do we need a ChangeLog entry for this?

valgrind and cppcheck had no complaints.

comment:7 in reply to: ↑ 6 Changed 7 years ago by tomek

  • Owner changed from tomek to tmark

Replying to tmark:

General question:

We have a class IOAddress (for better or worse), are we or should we have a class for IPv6Prefix? This is perhaps a larger picture question than this ticket's scope.

I think not. I defined such a class in Dibbler, but then almost never used it. The problem is that all existing functions expect IOAddress, not IOPrefix. Also, since the address part of the prefix uniquely identifies the whole prefix, it is sufficient to use just that. If you think about it, the extra information (prefix length) is almost never used for anything. getLease6() updateLease() etc. will all do fine without it.

Also it would complicate the interface a lot. Right now we have getLease6(type, address) that will eventually be able to grab normal addresses, temporary addresses and prefixes. The code for those 3 cases can be common. If we had implemented also getLease6(prefix), there would be code duplication.

So I really think it is not necessary.

option6_iaprefix.h

"The major differences are fields order and prefix has also additional prefix length field."
This doesn't quite make sense. Could you rephrase?

I did. Does it read better now?


option6_iaprefix.h

I have been told that Doxygen comments should use the word Constructor, not "Ctor" or "ctor".

Fixed.


option6_iaprefix.h

"It make everyone's like much easier,"

I think you meant:

"It makes everyone's life much easier"

Fixed.


option6_iaprefix.h

line spacing is "non-standard":

    /// @return string with text representation.
    virtual std::string
    toText(int indent = 0);

Fixed.


option6_iaprefix.h

pack and unpack methods can both throw but it is not mentioned in the header commentary.
In the case of the latter, a throw in unpack renders the contents of the object invalid,
the user should be made aware of this.

Added @throw notes about exceptions in couple methods. I hope I caught them all.

option6_iaprefix_unittests.cc

The unit testing only tests the "sunny path". It does not test invalid address in the packet,
which should case pack() to throw, or an invalid distance which should cause unpack() to throw.

Good point. The Option6IAPrefix(type, addr, length, pref, valid) was not tested either. Added 2 new tests. Since the option uses part of Option6IAAddr, I added one test there as well.

I noticed that IAAddr tests could be improved as well, but I don't want to bloat this ticket, so I only added @todo there.

Also, the constructor invocation that is there, should probably in an ASSERT_NO_THROW() because if it throws the test will blow up.

It is now ASSERT_NO_THROW. It doesn't make sense to continue if the constructor blew away.


option6_ia_unittests.cc

line 79 and 211, constructors should be within an ASSERT_NO_THROW(), yes?
line 85, pack() should also be in an ASSERT_NO_THROW?

Added.



Do we need a ChangeLog entry for this?

We do. See ChangeLog? file :) It was there, just waiting to be reviewed.

valgrind and cppcheck had no complaints.

Thanks for checking.

Ok, the ticket is back with you.

comment:8 Changed 7 years ago by tmark

  • Owner changed from tmark to tomek

Replying to tomek:

Replying to tmark:

General question:

We have a class IOAddress (for better or worse), are we or should we have a class for IPv6Prefix? This is perhaps a larger picture question than this ticket's scope.

I think not. I defined such a class in Dibbler, but then almost never used it. The problem is that all existing functions expect IOAddress, not IOPrefix. Also, since the address part of the prefix uniquely identifies the whole prefix, it is sufficient to use just that. If you think about it, the extra information (prefix length) is almost never used for anything. getLease6() updateLease() etc. will all do fine without it.

Also it would complicate the interface a lot. Right now we have getLease6(type, address) that will eventually be able to grab normal addresses, temporary addresses and prefixes. The code for those 3 cases can be common. If we had implemented also getLease6(prefix), there would be code duplication.

So I really think it is not necessary.

I'm good with this, it was really just a question on my part.

option6_iaprefix.h

"The major differences are fields order and prefix has also additional prefix length field."
This doesn't quite make sense. Could you rephrase?

I did. Does it read better now?

It does.


option6_iaprefix.h

I have been told that Doxygen comments should use the word Constructor, not "Ctor" or "ctor".

Fixed.


option6_iaprefix.h

"It make everyone's like much easier,"

I think you meant:

"It makes everyone's life much easier"

Fixed.


option6_iaprefix.h

line spacing is "non-standard":

    /// @return string with text representation.
    virtual std::string
    toText(int indent = 0);

Fixed.


option6_iaprefix.h

pack and unpack methods can both throw but it is not mentioned in the header commentary.
In the case of the latter, a throw in unpack renders the contents of the object invalid,
the user should be made aware of this.

Added @throw notes about exceptions in couple methods. I hope I caught them all.

option6_iaprefix_unittests.cc

The unit testing only tests the "sunny path". It does not test invalid address in the packet,
which should case pack() to throw, or an invalid distance which should cause unpack() to throw.

Good point. The Option6IAPrefix(type, addr, length, pref, valid) was not tested either. Added 2 new tests. Since the option uses part of Option6IAAddr, I added one test there as well.

I noticed that IAAddr tests could be improved as well, but I don't want to bloat this ticket, so I only added @todo there.

Also, the constructor invocation that is there, should probably in an ASSERT_NO_THROW() because if it throws the test will blow up.

It is now ASSERT_NO_THROW. It doesn't make sense to continue if the constructor blew away.


option6_ia_unittests.cc

line 79 and 211, constructors should be within an ASSERT_NO_THROW(), yes?
line 85, pack() should also be in an ASSERT_NO_THROW?

Added.



Do we need a ChangeLog entry for this?

We do. See ChangeLog? file :) It was there, just waiting to be reviewed.

Ah, so it was. I think perhaps adding mention of the new class would be good.

valgrind and cppcheck had no complaints.

Thanks for checking.

Ok, the ticket is back with you.

The ticket is good to go.

comment:9 Changed 7 years ago by tomek

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

Thanks for the review. Ticket merged.

Note: See TracTickets for help on using tickets.