BIND 10 #3082: Implement DHCPv4 Client FQDN option (81)
BIND 10 Development
do-not-reply at isc.org
Thu Aug 15 11:48:09 UTC 2013
#3082: Implement DHCPv4 Client FQDN option (81)
-------------------------------------+-------------------------------------
Reporter: marcin | Owner:
Type: enhancement | marcin
Priority: medium | Status:
Component: dhcp-ddns | reviewing
Keywords: | Milestone:
Sensitive: 0 | Sprint-DHCP-20130821
Sub-Project: DHCP | Resolution:
Estimated Difficulty: 0 | CVSS Scoring:
Total Hours: 0 | Defect Severity: N/A
| Feature Depending on Ticket:
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by tmark):
* owner: tmark => marcin
Comment:
Replying to [comment:6 marcin]:
> Replying to [comment:4 tmark]:
> > option4_client_fqdn.h -
> >
> > 1. Check line length option4_client_fqdn.h - several lines in the
class brief are longer than 80 and wrap.
>
> Fixed.
>
> >
> > 2. I think it would help if the comment clarified this is size on
bytes,
> > ..."The size(bytes)" or "The size in bytes"
> > {{{
> > /// @brief The size of the fixed fields within DHCPv4 Client Fqdn
%Option.
> > }}}
>
> Fixed.
> >
> > 3. Insert "an" between "create" and "instance" in the following line:
> >
> > {{{
> > /// This constructor is used to create instance of the option which
will be
> > }}}
>
> Added.
>
> >
> > 4. Shouldn't your contructor briefs include an @throw? You do
mention,
> > "Otherwise, the constructuro wil throw an exception", but I was under
the
> > impression that if a method throws it needed an @throw and it should
list
> > the exception Class(es) possible.
>
> In many cases it is a rat hole. It is very hard to come up with the
complete list of exceptions being thrown by the function. Nevertheless, in
this particular case I do throw certain exceptions, so your point is good.
I added throws to the documentation.
>
> >
> > 5. setDomainName method can throw, and there's no metion of that in
its brief.
>
> Added.
>
> >
> > 6. Why do we need a class for Rcode?
> >
>
> I am trying to make the interface easy to use and hard to misuse. The
valid values for Rcode are 0 and 255. By defining the objects which
encapsulate the valid values I encourage the caller to use only those
values that I want him to use. If I used other type (e.g. unit_8) it
assumes that I can pass any number from the range of 0..255. That is not
what I want. Also, please consider this constructor:
> {{{
> explicit Option4ClientFqdn(const uint8_t flags,
> const uint8_t rcode,
> const std::string& domain_name,
> const DomainNameType domain_name_type =
FULL);
>
> }}}
>
> It is quite easy to confuse first two parameters. In other words if you
pass the rcode value via flags field, and the flags via rcode, compiler
will not complain and everything will look ok, until you get some runtime
failure - which you have to investigate.
>
> Using my approach:
> {{{
> explicit Option4ClientFqdn(const uint8_t flags,
> const Rcode& rcode,
> const std::string& domain_name,
> const DomainNameType domain_name_type =
FULL);
>
> }}}
> there is no need to swap values (by mistake), you will get notified
about it in the compilation time.
This last argument sold me on the idea. What I would suggest you do is
add some commentary about why the class exists. Otherwise I think people
will go searching from some hidden or deeper reason, other than clarity.
I know that I did.
>
> >
> >
> > option4_client_fqdn.cc -
> >
> > 1. Check line length here too.
>
> Fixed.
>
> >
> > 2. Even though Option4ClientFqdnImpl is defined in the .cc file and
not the .h,
> > and its the imple part of PIMPL, the class defintion has no
commentary at
> > all. It defines 4 methods that Option4ClientFqdnImpl does not, some
of which
> > throw but you can't tell that unless you look at the code for each. I
would
> > think at least the methods unique to the Impl could use a header.
>
> I treated that as implementation detail. But, yes, this class is
documented in doxygen too, so I documented it.
>
Thank you. I just asked myself, "Would Marcin let me get away with not
documenting such a thing"? That answer was
"No." ;)
Oh, this line seems to missing 1 "/"
{{{
+ // Holds RCODE1 and RCODE2 values.
}}}
> >
> > 3. Option4ClientFqdn::getFlag()
> >
> > You are testing that the parameter flag only be valid enum value. It
can't be
> > anything but a valid value since the parameter type is Flag. It
shouldn't be
> > possible to pass it a value of 0x03. Isn't that the whole point of
making them
> > an enum set to begin with?
>
> We had a discussion on the chatroom. I think it is now clear. However, I
changed the data type to uint8_t so the confusion should be gone now. :-)
This is cleaner.
>
> >
> >
> > option4_client_fqdn_unittest.cc -
> >
> > 1.Rather that use the exact same names as the enums for the flags,
maybe you
> > should alter them a bit, like UBYTE_FLAG_S, or FLAG_S_BTYE? or
BFLAG_S..
>
> I thought that FLAG_X naming, together with the additional documentation
should be enough. is it? !''FLAG!'' refers to the name of the field in the
option, !''X!'' is a bit name as per RFC4703. I don't see any issue here.
>
> >
> > Maybe you should simple delcare them as uint8_t consts to begin with
as your
> > main line code doesn't really make much use of them being enums.
>
> I still believe that there are benefits of using enums - which makes
some parts of the interface more clear. On reflection, I don't care so
much. I made this change for either V6 or V4.
>
> >
> > 2. constructFromWire()
> >
> > You can initialize the array with chars if you want, so instead of
this:
> >
> > 6, 109, 121, 104, 111, 115, 116, // myhost.
> > 7, 101, 120, 97, 109, 112, 108, 101, // example.
> > 3, 99, 111, 109, 0 // com.
> >
> > you have this:
> >
> > 6, 'm', 'y', 'h', 'o', 's', 't', // myhost
> > 7, 'e', 'x', 'a', 'm', 'p', 'l', 'e', // example.
> > 3, 'c', 'o', 'm', 0 // com.
> >
> >
>
> You may be right. I just thought that the uint8_t is a numeric type. In
most (all?) cases it is implemented as:
> {{{
> typdef uint8_t char;
> }}}
> But, is it always the case? Will it cast the char value to uint8_t. If
you have strong opinion on this, I can submit another ticket. I don't want
to mess up here.
This was only a suggestion that would save you from having to look up the
ASCII codes, unless your a total Geek and have committed the chart to
memory.
I am relatively certain this would compile safely anywhere, but again it
was
just a suggestion.
Add the comments suggested and I do not need to see the ticket again.
--
Ticket URL: <http://bind10.isc.org/ticket/3082#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list