BIND 10 #3007: Develop NameChangeRequest class

[BIND 10 Development]

#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:8 marcin]:
 > 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.

 This time I REALLY got it.

 >
 >
 > <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

 So they do. Fixed all over.


 >
 > <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.

 Fixed.
 >
 > <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.

 Sorry, I did one, and then got distracted.

 >
 > > >
 > > > 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.

 Fixed.

 >
 > >
 > >
 > > > 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.

 I agree. Should have done it that way the first time. Have changed it to
 pointerless approach.
 >
 >
 >
 > >
 > > > 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);
 >     }
 > }
 > }}}


 I structured so it would tell you which one it failed on.
 I have changed to use your structure and the "<<" for the additional
 diagnostics.


 >
 >
 > 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.

 Sorry, I have to remember to double check for those.

 >
 > 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
 > }}}
 >

 Issues with ncr_msg.cc and ncr_msg.h are corrected.  There are others for
 d2_ files, which I'll address as they are worked.
 I do not want to mix them in with this ticket.  Now that I know about
 this, I'll do it going forward.

-- 
Ticket URL: <https://bind10.isc.org/ticket/3007#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development