BIND 10 #3007: Develop NameChangeRequest class
BIND 10 Development
do-not-reply at isc.org
Thu Jun 27 14:24:03 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:
Replying to [comment:7 tmark]:
> 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?
There is no point now. In the light of what you said here, it seems I
could have called d2_update_message class UpdateMessage or DNSUpdate. I
think understood wrong the naming conventions from our discussions on
jabber.
>
> Your d2_update_message.* could easily have been called dns_update.*, as
they really are not specific to DHPC-DDNS.
Agree.
>
>
>
> > !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.
IMO, the point of enums is to have named (instead of raw ints) values. If
you need an additional state meaining !''uninitialized!'' you can always
introduce the relevant value, say UNINITIALIZED.
<snip>
> >
> > a private member: !''brief!'' tag should start with lower case letter
>
> Oops again.
>
It is still there - see line 104.
<snip>
> > 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.
>
There is. If you don't intend to get arguments modified by your function,
declaring them const guarantees that it is caught in the compilation time
if you do. I agree it is not as big important as in case of references
but... what is the point in not declaring them const if they are not
modified?
Also, conding guidelines mandate it:
http://bind10.isc.org/wiki/CodingGuidelines#Consts
<snip>
> > 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.
Fair enough.
>
> Again, why const a primitive?
See above.
<snip>
> >
> > 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.
>
Hm.... that's fair enough, but I don't recall such a comment from Stephen
with regards to any other code. But, this is a minor issue, so I leave it
up to you.
<snip>
> > 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.
>
<snip>
> > '''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.)
Perhaps it is slower, but when programming in C++ there is always a
tradeoff between complexity and performance. Utilities are implemented and
we use them in various places of the code to make code simpler. If they
are slow, we could modify them rather than duplicate functionality.
<snip>
> >
> > NameChangeRequest::toFormat: There should be no new line between
!''switch (format)!'' and !''{!''
This was not addressed.
> >
> > 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.
>
That was just a suggestion. There are certain pieces of review which can
be address optionally if there is a reason for it. Overkill may be a good
reason to NOT do something :-)
>
> > 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.
@TODO should be replaced with @todo, otherwise doxygen will complain.
>
>
> > 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.
>
Sorry, I have learnt that from the Coding Guidelines where they use the
term !''curly braces!''. But it doesn't explain why I referred to it as
!''brackets!'' :-)
<snip>
> > '''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.
Changes look ok. On reflection, I think it would be even better not to use
pointers at all:
{{{
EXPECT_THROW(NameChangeRequest(CHG_ADD, true, true, "",
"192.168.1.101", dhcid, expiry, 1300),
NcrMessageError);
}}}
instead of:
{{{
EXPECT_THROW(ncr.reset(new NameChangeRequest(CHG_ADD, true, true, "",
"192.168.1.101", dhcid, expiry, 1300)), NcrMessageError);
}}}
but I am also fine with the current solution.
>
> > 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.
Neat pick: There should be spaces sourrounding !''+!'' in std::equal.
<snip>
>
> > 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)
>
>
>
The !ChangeLog looks fine.
I apologize, I missed those in the previous round of review...
invalidMsgChecks, validMsgChecks: Why not do the following:
{{{
TEST(NameChangeRequestTest, invalidMsgChecks) {
// Iterate over the list of JSON strings, attempting to create a
// NameChangeRequest. The attempt should throw a NcrMessageError.
int num_msgs = sizeof(invalid_msgs)/sizeof(char*);
for (int i = 0; i < num_msgs; i++) {
EXPECT_THROW(NameChangeRequest::fromJSON(invalid_msgs[i]),
NcrMessageError);
}
}
}}}
Also, please check whitespaces in: ncr_msg.h and ncr_msg_unittests.cc.
There are spurious spaces at the end of line in a few places.
The following doxygen errors are reported:
{{{
/home/marcin/devel/bind10/src/bin/d2/ncr_msg.cc:69: warning: no matching
class member found for
isc::d2::NameChangeRequest::NameChangeRequest(isc::d2::NameChangeType
change_type, bool forward_change, bool reverse_change, const std::string
&fqdn, const std::string &ip_address, const isc::d2::D2Dhcid &dhcid, const
ptime &lease_expires_on, uint32_t lease_length)
Possible candidates:
isc::d2::NameChangeRequest::NameChangeRequest()
isc::d2::NameChangeRequest::NameChangeRequest(NameChangeType
change_type, bool forward_change, bool reverse_change, const std::string
&fqdn, const std::string &ip_address, const D2Dhcid &dhcid, const
boost::posix_time::ptime &lease_expires_on, uint32_t lease_length)
/home/marcin/devel/bind10/src/bin/d2/ncr_msg.cc:402: warning: no matching
class member found for
void isc::d2::NameChangeRequest::setLeaseExpiresOn(const ptime &value)
Possible candidates:
void isc::d2::NameChangeRequest::setLeaseExpiresOn(const
boost::posix_time::ptime &value)
void isc::d2::NameChangeRequest::setLeaseExpiresOn(const std::string
&value)
void
isc::d2::NameChangeRequest::setLeaseExpiresOn(isc::data::ConstElementPtr
element)
...
/home/marcin/devel/bind10/src/bin/d2/ncr_msg.h:137: warning: argument
'ptime' of command @param is not found in the argument list of
isc::d2::NameChangeRequest::NameChangeRequest(NameChangeType change_type,
bool forward_change, bool reverse_change, const std::string &fqdn, const
std::string &ip_address, const D2Dhcid &dhcid, const
boost::posix_time::ptime &lease_expires_on, uint32_t lease_length)
/home/marcin/devel/bind10/src/bin/d2/ncr_msg.h:137: warning: The following
parameters of isc::d2::NameChangeRequest::NameChangeRequest(NameChangeType
change_type, bool forward_change, bool reverse_change, const std::string
&fqdn, const std::string &ip_address, const D2Dhcid &dhcid, const
boost::posix_time::ptime &lease_expires_on, uint32_t lease_length) are not
documented:
parameter 'lease_expires_on'
/home/marcin/devel/bind10/src/bin/d2/ncr_msg.h:348: warning: argument
'string' of command @param is not found in the argument list of
isc::d2::NameChangeRequest::setDhcid(const std::string &value)
/home/marcin/devel/bind10/src/bin/d2/ncr_msg.h:348: warning: The following
parameters of isc::d2::NameChangeRequest::setDhcid(const std::string
&value) are not documented:
parameter 'value'
/home/marcin/devel/bind10/src/bin/d2/ncr_msg.h:229: warning: Found unknown
command `\TODO'
/home/marcin/devel/bind10/src/lib/datasrc/sqlite3_accessor.cc:899:
warning: The following parameters of
isc::datasrc::SQLite3Accessor::DiffContext::DiffContext(const
boost::shared_ptr< const SQLite3Accessor > &accessor, int zone_id,
uint32_t start, uint32_t end) are not documented:
parameter 'accessor'
...
}}}
In order to reproduce it:
{{{
cd doc
make devel
less html/doxygen-error.log
}}}
--
Ticket URL: <http://bind10.isc.org/ticket/3007#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list