BIND 10 #1144: RR type implementation: DLV

[BIND 10 Development]

#1144: RR type implementation: DLV
-------------------------------------+-------------------------------------
                   Reporter:  shane  |                 Owner:  jinmei
                       Type:  task   |                Status:  reviewing
                   Priority:  major  |             Milestone:
                  Component:         |  Sprint-20110816
  DNSPacket API                      |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DNS
Feature Depending on Ticket:         |  Estimated Difficulty:  3
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 - 'make distcheck' failed for me.  Please check and fix it.  In
   general, when your branch contains a new file I'd suggest running
   distcheck before asking for review.  From my past experiences, we
   often forget making necessary changes to Makefile.am regarding the newly
   introduced file, which doesn't affect normal make but makes
   distcheck fail.

 - is it a good idea to use inheritance to (re)define DS and DLV?  why
   not just define them from the template?  That is, can't we say
   something like this? (but see below before responding to this
   specific point)
 {{{
 typedef DS_LIKE<DLV, 32769> DLV;
 typedef DS_LIKE<DS, 43> DS;
 }}}

 - this probably doesn't work as expected
 {{{
 #include <dns/rdata/generic/detail/ds_like.h>
 }}}
   because header files under rdata/ are not intended to be installed.
   (but see below before responding to this specific point)

 - the previous point may make this a no issue, but we cannot use
   'using namespace' in ds_like.h (just like we cannot do it in public
   header files).  (but see below before responding to this
   specific point)

 - noticing these three things above and browsing the entire
   implementation, I'd suggest the following approach:
   - declare DS and DLV just like others (no template, no inheritance)
   - both have a common "DSLikeImpl" class
   - forward (almost) everything in DS and DLV to the implementation class:
 {{{
   DS(InputBuffer& buffer, size_t rdata_len) : impl_(new DSLikeImpl(buffer,
 rdata_len)) {}
   string DS::toText() const { return (impl_->toText()); }
 }}}
   etc.  the copy constructor and operator= may have to be defined
   separately, resulting in some redundancy, but I think it's
   acceptable because these would just be an application of a common
   idiom and won't change much once written.
   - define DSLikeImpl based on the current DS_LIKE definitions.
     DSLikeImpl won't have to be templated because most of it don't
     depend on class specific information.  The only non trivial part
     (so far) is compare(), for which I'd define a template function
     and call it from both the DS and DLV versions.
   - all implementation details are hidden inside a .cc (and possible
     in a .h only included from that .cc, just like we did for NSEC and
     NSEC3)

 - what's the purpose of DS::id?
 {{{
 class DS : public DS_LIKE<DS, 43> {
 ...
     static string const id;
 }}}
   it doesn't seem to be used (and it compiled even if I commented out
   this line).  Same for DLV::id.  BTW, if we really need this, we
   should be explicit about the standard namespace, i.e., 'string'
   should be 'std::string' as this is a header file included in a .cc.

 - what's the purpose of "typeCode" template parameter of DS_LIKE?  It
   doesn't seem to be used at all.

 - like the above two, this may not be an issue depending on how we
   resolve the bigger point, but why did you explicitly define some
   class methods as inline explicitly instead of defining them within
   the class declaration?  i.e., instead of doing:
 {{{
 inline DS::DS(isc::util::InputBuffer& buffer, size_t rdata_len) :
 DS_LIKE<DS, 43>(buffer, rdata_len) {}
 }}}
   (btw this line is too long) we could:
 {{{
 class DS {
 ...
     DS(isc::util::InputBuffer& buffer, size_t rdata_len) :
         DS_LIKE<DS, 43>(buffer, rdata_len)
     {}
 };
 }}}
   The former is not necessarily wrong or bad, but I thought the latter
   style is more common, and is at least more consistent with other
   part of BIND 10 code.

 - I've noticed some style nits (some too long lines, missing
   parenthesis, etc), but I'll defer these to the next round of review
   because the previous points would require substantial rewrite
   anyway.

 - We need comments to the implementations (when we first wrote the DS
   class we didn't have time toward the first official release and made
   a compromise).  Now that we don't have this excuse the RDATA
   implementations are no different from others on that point.  See the
   TSIG class for example doc.

 - We'll need tests for DLV even if we are quite sure if it works when
   DS works.  Ideally we can share the test code with templates or
   something.

 - I guess we need a changelog entry for this task.

-- 
Ticket URL: <http://bind10.isc.org/ticket/1144#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development