BIND 10 #1575: move data_source.cc:Nsec3Param to libdns++

[BIND 10 Development]

#1575: move data_source.cc:Nsec3Param to libdns++
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120207
  critical                           |            Resolution:
                  Component:         |             Sensitive:  0
  libdns++                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  4
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:  NSEC3  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:6 vorner]:

 Thanks for the review.

 > > I think it's worth a changelog entry.  This is the proposed one:
 > >
 > >     libdns++: a new class NSEC3Hash was introduced as a utility ofn
 >
 > I believe there's a typo (should it be utility *for*…?).

 Right, corrected the trac comment directly (so I can simply copy-paste
 it later).

 > Does Doxygen generate the „brief“ descriptions from the first paragraph
 automatically? It seems so, but I'd prefer having them there, just for the
 clarity.

 Yes, it does, but I have no objection to explicitly adding it for
 clarity.  Done.  (If that's our consensus we should probably document
 it as part of guideline).

 > The word „alphabets“ looks wrong, I believe an alphabet is a set of
 characters a language uses to write its words (like Czech alphabet, Greek
 alphabet, …). Would „letters“ be better? (both in C++ and python docs)

 Okay, I used "US-ASCII letters", borrowing from RFC4034 and RFC5155.

 > The error message „function must be given NSEC3HASH Rdata“ is possibly
 correct according to English rules, but it took me like 5 seconds to parse
 it, as the nominative is at the end. Maybe something „You must provide a
 NSEC3HASH Rdata to the function“.

 Hmm, I'm okay with revising the message, but in that case I'd rather
 be more consistent with messages for this type of error from other
 Python built-in libraries.  I've changed them that way.

 > I'm little bit worried about the future extendability of this class.
 Would it be better to have a pointer (shared pointer, or whatever)
 returned from a static function? That way each algorithm could have its
 own concrete implementation of the class, if needed, and the function
 could decide which one should be created. This would also remove the need
 for having a private implementation class (!NSEC3HashImpl), as the
 concrete class could be effectively hidden. I understand this
 implementation is temporary, as good for now and we don't need to support
 eg. md5. But I'd propose at last changing the interface (to have the
 function be virtual and allocation being done by the static function), so
 we can keep it when we revisit it and generalise it.

 Do you mean introducing a base class and defining a factory function
 (or method) like this?

 {{{#!c++
 class NSEC3HashBase {
 public:
     static NSEC3HashBase* create(const rdata::generic::NSEC3PARAM& param);
     virtual std::string calculate(const Name& name) const = 0;
 };
 }}}
 (maybe we could name the base class `NSEC3Hash` and hide the concrete
 one in the implementation).

 I'm okay with that, and, in fact, as commented in the documentation I
 expect we'll probably want to allow tests to define and use fake
 derived class so it can hardcode hash values.  This framework will be
 used for that purpose, too.

 But I'm not sure if we want to define derived classes for different
 algorithms.  Again, as noted, my not-so-fixed intention is to allow
 the application to switch parameters (including the algorithm) while
 keeping the hash object itself so it can reuse the internal resource
 as long as possible.  That would be possibly useful for validator,
 which would create and hold a single `NSEC3Hash` object and switch the
 parameter for each validation attempt.

 > Also, every time I see the python wrapper file generated form the
 template, I wonder if we should leave the comments about what should be
 done to fill in the template in, as this looks as something was forgotten
 to be done. But that might be for a wider discussion than just this
 ticket.

 Are you specifically talking about this one (for this branch)?

 {{{#!c++
 // This is a template of typical code logic of python class initialization
 // with C++ backend.  You'll need to adjust it according to details of the
 // actual C++ class.
 }}}

 This should have been removed, and I did it.  I tried to remove this
 type of comments in the branch, but this one was overlooked.  Is there
 anything else you noted?

 As a general issue, it may help if we mark such comments in the
 template like this:
 {{{#!c++
 // THIS BLOCK OF COMMENT MUST BE REMOVED FROM THE PUBLISHED VERSION
 // This is a template of typical code logic of python class initialization
 // with C++ backend.  You'll need to adjust it according to details of the
 // actual C++ class.
 }}}

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