BIND 10 #3007: Develop NameChangeRequest class
BIND 10 Development
do-not-reply at isc.org
Wed Jun 26 17:22:23 UTC 2013
#3007: Develop NameChangeRequest class
-------------------------------------+-------------------------------------
Reporter: tmark | Owner: tmark
Type: enhancement | Status:
Priority: medium | reviewing
Component: dhcp-ddns | Milestone:
Keywords: | Sprint-DHCP-20130703
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 marcin):
* owner: marcin => tmark
Comment:
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?
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)
}}}
'''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?
!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.
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?
Invalid alignment of the !''public!'' and !''private!'' keywords
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.
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.
Also, typo in the doxygen documentation of the destructor - missing
!''brief!''.
fromStr: spurious space between argument type and ampersand.
a private member: !''brief!'' tag should start with lower case letter
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.
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.
NameChangeRequest constructor: Theoretically, all constructor arguments
could be declared const because constructor does not modify them (it
probably shouldn't).
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.
NameChangeRequest destructor: this class does not contain any virtual
functions, therefore destructor should not be virtual either.
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.
NameChangeRequest::toFormat: format could be declared const and have a
default value.
Also, can this function be declared const? I believe it should not modify
class members.
NameChangeRequest::toJSON: This function could be declared const because
it does not modify the class members.
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.
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.
NameChangeRequest::getChangeType could be declared const.
NameChangeRequest::getElement: can this function be declared const?
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.
private member lease_expires_on_: in the doxygen description the
!''brief!'' tag and its text should be separated from the detailed
description.
isReverseChange and isForwardChange: why is the return type marked const?
This is BTW causing compilation issues on g++ on Linux.
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
'''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.
NameChangeRequest::fromFormat: there should be no new line between
!''switch (format)!'' and !''{!''.
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());
}
}}}
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.
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.
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)) {
}}}
NameChangeRequest::toText, the !''chgAdd!'' and !''chgRemove!'' should be
renamed to upper case once enum values' names are renamed.
'''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.
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);
}}}
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.
Can you please suggest a !ChangeLog entry?
--
Ticket URL: <http://bind10.isc.org/ticket/3007#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list