BIND 10 #1455: Add prerequisite checks to the DDNS module

[BIND 10 Development]

#1455: Add prerequisite checks to the DDNS module
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  jelte
                       Type:  task   |                Status:  reviewing
                   Priority:         |             Milestone:
  medium                             |  Sprint-20120529
                  Component:  DDNS   |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DNS
Feature Depending on Ticket:  DDNS   |  Estimated Difficulty:  6
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jelte


Comment:

 Hello

 I don't see any substantial problem, but I see several small issues, as
 always with such a huge branch.

 '''Aesthetic things'''

 In several cases, there's a space at the end of line. Could I recommend
 configuring your editor to highlight them in red? I have the configuration
 for vim somewhere and I believe Jinmei sent one for emacs as well. Here is
 the concrete list:

 {{{#!c++
     addClassVariable(name_comparison_result_type, "COMMONANCESTOR",
                      Py_BuildValue("I",
 NameComparisonResult::COMMONANCESTOR));
     ← Space here
 }}}

 {{{#!c++
         }
         ← Space here
         s += getName().toText() + " " + getTTL().toText() + " " +
 }}}

 {{{#!python
         # This test checks if the actual prerequisite-type-specific
         # methods are called.
         # It does test all types of prerequisites, but it does not test
         # every possible result for those types (those are tested above,
         # in the specific prerequisite type tests)
         ← Space here
         # Let's first define a number of prereq's that should succeed
 }}}

 When we talk about code style and similar, this description confuses me.
 What is the `''` thing? It is in the `LIBDDNS_PREREQ_RRSET_EXISTS_FAILED`
 and `LIBDDNS_PREREQ_RRSET_EXISTS_VAL_FAILED` messages.

 {{{
 In this case, the specific prerequisite is ''. From RFC2136:
 }}}

 And there's a typo (itw):
 {{{#!python
         # but just for better failures, it is first called on itw own
 }}}

 '''The actual code'''

 As I mentioned in the review of #1512, I consider the thing with delayed
 conversion (`RRsetFormatter` in this case) to string as a premature
 optimisation. This is python and concatenating two strings is pretty cheap
 operation, relative to other things. I fear that the allocation of the
 temporary object that will or will not be converted to the string and its
 initialization may be if not more expansive than the conversion, than at
 least comparably expensive. Therefore I don't really like the creation of
 classes whose only point of existence is to delay something. I suggested
 to Jinmei that a comment saying that it is meant for optimization, but we
 don't really know if it helps would be enough, but I don't want more of
 these things to be appearing in our code.

 I also worry little bit about performance of using `find` or `find_all` to
 check if a name exists. It has some pretty complex handling and the `_all`
 one copies (and wraps to python) potentially a lot of RRsets.

 Also, in the search if actual Rdata match, you remove Rdata from the
 RRSet. Is it safe? Can't it be the same RRset that is being preserved
 inside the In-Memory data source? In such case, the removals would
 actually destroy the RRset effectively. Would it be possible to just sort
 the Rdata by their textual representation (if they are not comparable
 directly)?

 '''The tests'''

 First off, there are two separate copies of very similar code, testing the
 positive and negative prerequisite checks. Could these be unified to a
 single function, called with a boolean (which would specify if the answer
 should be True or False) and the tested function? I think having so much
 duplicate code signals a problem.

 All the checks with `A/MX` is being done with empty RRsets. Is it OK? I
 mean, you added support for empty RRsets, but these are neither class
 `ANY`, nor `NONE`, so this setup should be kind of illegal.

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