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

[BIND 10 Development]

#1455: Add prerequisite checks to the DDNS module
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  vorner
                       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 jelte):

 * owner:  jelte => vorner


Comment:

 Replying to [comment:12 vorner]:
 > 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:
 >

 i use neither, my editor does show them, but apparently not clearly enough
 ;)

 anyway, fixed.

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

 It signals that I have forgotten to put in the name of the prerequisite as
 taken from the rfc (like 'Name is in use'), and as used in the message
 itself. Fixed.

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

 ack, fixed

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

 A better way to put it would be 'string concatenation is expensive, but so
 is everything' :) (especially when doing multiple concats which is not the
 case here, but I digress). Good point. The formatters themselves (even if
 they were static functions that immediately performed the operation) are
 nice though, to get uniform output. We should probably move them to util
 though.

 For the sake of consistency I'll take over whatever 1512 ends up doing :)

 In this specific case overly applying optimization for the case where
 logging does not occur is probably even more superfluous; these messages,
 if encountered, would almost always be logged anyway.

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

 Are you suggesting we should have better find methods? (for instance
 similar ones to these, but only signal back that they *would* return
 something if the real version was called?)

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

 Hmm. Given that you were just worried about performance, doing to-text and
 sorting would probably not be very efficient either (then again, neither
 is this current algorithm). I don't *think* we can actually delete data
 this way, but a relatively cheap solution would be to make a shallow copy
 of the list we are deleting from (so it'll only mess with refcounts).

 The real solution would be to have builtin comparison of rrsets though :)
 It seems we need this every other week or so. I'm gonna make a ticket.

 (as a proposal i committed the shallow copy as the final commit, we can
 choose not to use that and go for something else)

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

 Ack. Well, this may cause a comparatively large diff, but once i had a
 more general method-passing way for those tests, the new generalized
 helper function would also make the other tests look better. I did not
 remove or add any tests, but did merge the several dupes, and the result
 looks nicer imo.
 (so hey this comment did cause more change than expected :P maybe it's
 me.)

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

 You are right, it happens to work as they aren't actually put into packets
 or printed, but they should have their correct classes, fixed (and to make
 it even more complete, the rrclass is now also passed as an argument,
 since technically they should sometimes be ANY and sometimes be NONE,
 though the way it is implemented, it currently does not really matter)

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