BIND 10 #3007: Develop NameChangeRequest class
BIND 10 Development
do-not-reply at isc.org
Thu Jun 27 12:37:47 UTC 2013
#3007: Develop NameChangeRequest class
-------------------------------------+-------------------------------------
Reporter: tmark | Owner:
Type: enhancement | marcin
Priority: medium | Status:
Component: dhcp-ddns | reviewing
Keywords: | Milestone:
Sensitive: 0 | Sprint-DHCP-20130703
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]:
> Reviewed commit b4b1eca7131cedca72a8973718881c1f18d2010f
>
> '''General'''
> The naming if the class and files is not consistent with naming of other
files, which are prefixed with !''d2_!'' or !''d_!''. Can you please
clarify once again what the selected naming convention for the files in d2
is?
The "d_" are base classes independent of DHCP-DDNS, that actually form the
basis for a BIND10 module framework if you will.
The "d2_" are larger grained classes specific to DHCP-DDNS, that are
typically derived from the "d_" framework.
NameChangeRequest as a class name originates in the D2 design. I wanted
the file names to reflect the class. And while this message is specific
to DHCP-DDNS, its more along the lines of a "package" or library used by
D2. Besides if all of the source files started with "d2_"
then what would be the point?
Your d2_update_message.* could easily have been called dns_update.*, as
they really are not specific to DHPC-DDNS.
>
> Unit tests fail for me on Ubuntu with gcc 4.6:
> {{{
> [ RUN ] NameChangeRequestTest.invalidMsgChecks
> Invalid Message: Invalid data value for change_type: 7
> Invalid Message: Wrong data type for forward_change :boolValue() called
on non-Bool Element
> Invalid Message: Wrong data type for reverse_change :boolValue() called
on non-Bool Element
> Invalid Message: Invalid Request, forward and reverse flags are both
false
> Invalid Message: FQDN cannot be blank
> Invalid Message: Invalid ip address string for ip_address: xxxxxx
> Invalid Message: String to byte conversion, invalid data length: 0
> Invalid Message: String to byte conversion, invalid data length: 15
> Invalid Message: Invalid data in Dhcid
> Invalid Message not caught, message idx: 9
> ncr_unittests.cc:375: Failure
> Value of: it_threw
> Actual: false
> Expected: true
> Invalid Message: Wrong data type for lease_length: intValue() called on
non-integer Element
> [ FAILED ] NameChangeRequestTest.invalidMsgChecks (1 ms)
> }}}
That test should be catching an invalid ISO date-time string. Apparently
under Ubuntu the test string I chose:
\"19620121132405\"
,which is invalid under OS-X, is valid. I have replaced it with a string
that is definitely not a valid ISO date-time string.
\"Wed Jun 26 13:46:46 EDT 2013\"
>
>
> '''ncr_msg.h'''
> In the following code section:
> {{{
> #ifndef _NCR_MSG_H
> #define _NCR_MSG_H
> }}}
> prefix !''_!'' is used before defines. In general, this prefix is not
used in other files. is there any particular reason why it is used here?
>
That was a typo. Fixed it.
> !NameChangeType, !NameChangeStatus, !NameChangeFormat: enumerator type
values should be in upper case.
>
> Also, why do you initialize the first enum value to 1 explicitly? There
is usually no need to explicitly initialize enumerators unless you want to
iterate on them like on regular integers. On the other hand, even if this
is the case, this may be unsafe, because compilers would not guarantee
that the other enumerator values will be assigned: 2, 3, 4, etc.
>
Ok, I looked it up in the coding guidelines, we are both right and both
wrong.
Apparently they are suppose to be like this:
enum EnumName {
FOO,
BAR
} enum_instance;
and without specific values unless necessary. One of the reasons I like
non-zero values, is that the values look more intentional at run time. Is
it 0 or uninitialized? No matter, I'll adhere to the guideline.
> D2Dhcid: I am confused that class name is prefixed with D2, while
!''!NameChangeRequest!'' is not.
>
> I am wondering if the class which encapsulates DHCID should be in the
ncr_msg.h. It seems to be pretty generic class. So, if somebody just needs
to use this class, why would he pull the whole header to just use this
class?
>
> Perhaps, better name for this class would be D2dhcid, or DHCID?
There is a DHCID class in lib/dns, though it does far more than is needed
here. I was hesitant to use it. D2Dhcid is specifically for encapsulating
the value for residence within a NameChangeRequest. I am not certain is
warrants an existence outside of the ncr source files. I suppose I could
nest it with NameChangeRequest. Additionally, if it is later determined
the dns::DHCID is more appropriate, then the D2Dhcid goes away without
much fuss.
>
> Invalid alignment of the !''public!'' and !''private!'' keywords
>
Oops. Fixed it.
> Constructor: Is there any case when we want to create an object from the
binary value - e.g. vector holding bytes, which represent a DHCID? If not,
existing constructor may be sufficient.
There may be. If and when we support other formats. I would prefer to add
should the need arise.
>
> Destructor: This class provides virtual destructor but has no virtual
functions. Virtual destructor is required when there is at least one
virtual function. When there is no virtual function it means that class is
not meant to be a base class and using virtual destructor is a bad idea.
That is mainly because the class need to hold the virtual table which
increases its size.
Sometimes I get carried away. Gone.
>
> Also, typo in the doxygen documentation of the destructor - missing
!''brief!''.
>
n/a - see above
> fromStr: spurious space between argument type and ampersand.
Got it.
>
> a private member: !''brief!'' tag should start with lower case letter
Oops again.
>
> Copy constructor and assignment operator: I believe the DHCID is a
unique identifier so there should be no reason to copy the existing
object. In this case, the copy constructor and assignment operator could
be made private.
>
Could be, but there's not a compelling reason (yet) that I see to not have
them (yet).
>
> NameChangeRequest doxygen header: a text is messed up - one line runs on
another. Also, the text following !''brief!'' tag should be short (a sort
of header) and the detailed description should be separated by a new line.
>
Done.
> NameChangeRequest constructor: Theoretically, all constructor arguments
could be declared const because constructor does not modify them (it
probably shouldn't).
There is no point in declaring a primitive const.
>
> Typos in doxygen comments: s/indicates if this change should
sent/indicates if this change should be sent/
> s/fqdn is the domain name/the domain name/
> s/ip_address is the/ip_address the/
> etc.
>
Done.
> NameChangeRequest destructor: this class does not contain any virtual
functions, therefore destructor should not be virtual either.
>
Gone.
> NameChangeRequest::fromFormat: please consider providing default value
for the !''format!'' argument because it sounds that we will be using JSON
it most of the time. Also !''format!'' could be declared const.
>
I would prefer not to supply a default. Reading through code and seeing
this "fromFormat(FMT_JSON, some_buffer)" leaves no room to wonder about
the intent, compared to "fromFormat(ome_buffer)". It is better for the
interface to insist on clear intent from the developer.
Again, why const a primitive?
> NameChangeRequest::toFormat: format could be declared const and have a
default value.
See comments pertaining to fromFormat.
>
> Also, can this function be declared const? I believe it should not
modify class members.
Yes it can be. Good catch.
>
> NameChangeRequest::toJSON: This function could be declared const because
it does not modify the class members.
Yes it can be. Good catch.
>
> For both functions above: in the doxygen comments there is no need to
say:
> {{{
> @return returns a ...
> }}}
> I could be:
> {{{
> @return a
> }}}
> because the tag itself says that the comment is about returning a value.
Ok, but if I don't include them, Stephen will tell me they are missing.
>
> NameChangeRequest::validateContent: the doxygen comment lists rules
using numbers 1. 2. 3. This will look messed-up in doxygen. Consider using
!''dashes!'' instead. Alternatively, you could use html tags.
>
Ok, dashes.
> NameChangeRequest::getChangeType could be declared const.
>
Got it.
> NameChangeRequest::getElement: can this function be declared const?
>
Got it.
> private member fqdn_ and ip_address_: I am wondering if more specialized
type could be selected for these members. For example, for the fqdn_ it
could be !''isc::dns::Name!'', for the other one it could be
!''isc::asiolink::ip_address!''. They have the functionality to validate
whether the given value is a valid name or IP address.
>
Addressed below.
> private member lease_expires_on_: in the doxygen description the
!''brief!'' tag and its text should be separated from the detailed
description.
That detail was supposed to have been deleted, I placed the same
explanation up with the definition of TimePtr.
>
> isReverseChange and isForwardChange: why is the return type marked
const? This is BTW causing compilation issues on g++ on Linux.
Typo.
>
> std::string getLeaseExpiresOn: suggest that the doxygen header includes
the example of the string representation of the timestamp returned by this
function.
>
> setLeaseExpiresOn: should given an example of the timestamp in the
string format
>
Added.
> '''ncr_msg.cc'''
> D2Dhcid::fromStr: this implementation is overly complicated. Similar
problem is addressed in src/lib/dhcp/option_data_types.cc, function
writeBinary.
>
> D2Dhcid::toStr: In !''util::encode!'', there are suitable functions to
implement this easier.
>
Changed to use util::encode. (While the impl I had might have seemed
overly complicated it is extremely fast. I will bet you money that the
util::encode is much, much slower.)
> NameChangeRequest::fromFormat: there should be no new line between
!''switch (format)!'' and !''{!''.
Right, sometimes I forget we use K&R bracing, which is unfortunate.
>
> Suggest that the code is modified to utilize STL objects which are
safer; and, for example, do not require manual addition of NULL at the end
of string. Also, it is probably not a problem with recent versions o{{{
> if (raw_value != chgAdd && raw_value != chgRemove) {
> }}}
> f gcc, but I saw issues presented by some compilers when using variadic
arrays. If you concur, maybe you could consider a code similar to this:
>
> {{{
> try {
> // Get the length of the JSON text and create a char buf
large
> // enough to hold it + 1.
> size_t len = buffer.readUint16();
>
> std::vector<uint8_t> vec;
> buffer.readVector(vec, len);
> std::string string_data(vec.begin(), vec.end());
>
> // Pass the string of JSON text into JSON factory to create
the
> // NameChangeRequest instance. Note the factory may throw
> // NcrMessageError.
> ncr = NameChangeRequest::fromJSON(string_data);
> } catch (isc::util::InvalidBufferPosition& ex) {
> // Read error accessing data in InputBuffer.
> isc_throw(NcrMessageError, "fromFormat: buffer read error: "
> << ex.what());
> }
> }}}
Leave it to C++ compilers to ruin VLAs. Done.
>
> NameChangeRequest::toFormat: There should be no new line between
!''switch (format)!'' and !''{!''
>
> NameChangeRequest::toJSON: Instead of accesing class members directly
(such as change_type_, forward_change_ etc), it would be better to use
appropriate accessors of the class. Currently, they are one-liners so they
will probably be inlined by the compiler and thus it should not affect
performance.
>
Overkill, but if it makes you happy.
> NameChangeRequest::validateContent: The check on FQDN is not sufficient.
The FQDN has a defined structure which should be followed. For example,
something like !''example...com!'' is not a valid FQDN but will pass this
function. I would rather suggest to use the !''isc::dns::Name!'' class to
validate FQDNs. If it is however intended that the detailed check is done
on the DnsClient level, before sending a packet to DNS, it should be noted
as a comment. Nevertheless, I believe it should be well validated here.
>
> Although, I haven't had time to go into details of DHCID, the RFC 4701
presents certain formats of DHCID. It may be good for now to check that
the DHCID is not empty, but ultimately, we would like to have a DHCID
which would be capable to verify that DHCID is valid or at least certain
part of it is valid. Please consider adding a suitable @todo comment into
this function.
>
>
I agree that there is more validation that we could and should do. I had
considered using dns:DHCID but it carries a long a lot of extra
functionality and In the interests of getting D2 up on its feet I
deferred more detailed decisions like this and opted for a somewhat
lighter path.
I will mark using Name and DHCID and expanding validation with TODOs.
There is still a great deal of work to do and there are larger concerns at
the moment.
> NameChangeRequest::setChangeType: Some compilers issue warnings when
curly brackets are not used in expressions like this one:
> {{{
> if (raw_value != chgAdd && raw_value != chgRemove) {
> }}}
>
> Please consider this:
> {{{
> if ((raw_value != chgAdd) && (raw_value != chgRemove)) {
> }}}
Done. BTW, those are not "curly brackets" they are parens.
>
> NameChangeRequest::toText, the !''chgAdd!'' and !''chgRemove!'' should
be renamed to upper case once enum values' names are renamed.
>
Done.
> '''ncr_unittests.cc'''
> NameChangeRequestTest::constructionTests: this whole test is memory leak
prone. Tests are passing if constructors throw exceptions. However, please
consider the following scenario:
>
> ''Suppose there is a bug in implementation of the constructor which
allows blank FQDNs. In this case the following call:
> {{{
> EXPECT_THROW(
> new NameChangeRequest(chgAdd, true, true, "", "192.168.1.101",
dhcid, expiry, 1300),
> NcrMessageError
> )
> }}}
> will not trigger exception and the object will be successfully created!
> Test will happily proceed to the next check, leaving the object on the
heap. You can simulate such scenario by changing FQDN to non-empty value
for a moment and running unit test through valgrind. The memory leak is
reported.
> In order to prevent it, please consider using boost::scoped_ptr to
automatically release the object.
>
Ok, I see your point and I have done as suggested. I wasn't that mindful
with it being unit test code.
> NameChangeRequestTest::dhcidTest: a slightly clearer way to initialize
!''expected_bytes!'' could be:
> {{{
> const uint8_t bytes[] = { 0x1, 0x2, 0x3, 0x4, 0xA, 0x7F, 0x8E, 0x3D };
> std::vector<uint8_t>(bytes, bytes + sizeof(bytes) - 1);
> }}}
Oy vey.
>
> Please consider using std::equal to compare two vectors. Relying on the
!''=!'' operator when using EXPECT_EQ used to cause odd issues on some
systems.
>
Done. Nice to know you can't rely on anything.
> Can you please suggest a !ChangeLog entry?
ChangeLog:
xxx [func] [tmark]
Added initial implementation of NameChangeRequest,
which embodies DNS update requests sent to DHCP-DDNS
by its clients.
(trac3007 git TBD)
--
Ticket URL: <http://bind10.isc.org/ticket/3007#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list