BIND 10 #3082: Implement DHCPv4 Client FQDN option (81)
BIND 10 Development
do-not-reply at isc.org
Wed Aug 14 18:51:58 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
-------------------------------------+-------------------------------------
Comment (by 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.
>
>
> 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.
>
> 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. :-)
>
>
> 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.
--
Ticket URL: <http://bind10.isc.org/ticket/3082#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list