BIND 10 #3145: PD: Add support for IA_PD and IAPREFIX options

[BIND 10 Development]

#3145: PD: Add support for IA_PD and IAPREFIX options
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp6         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130918
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * owner:  tmark => tomek


Comment:

 Replying to [comment:7 tomek]:
 > Replying to [comment:6 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.

-- 
Ticket URL: <https://bind10.isc.org/ticket/3145#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development