BIND 10 #181: python wrappers to support xfr

[BIND 10 Development]

#181: python wrappers to support xfr
----------------------+-----------------------------------------------------
 Reporter:  larissas  |        Owner:  jelte                                         
     Type:  task      |       Status:  reviewing                                     
 Priority:  major     |    Milestone:  05. 3rd Incremental Release: Serious Secondary
Component:  xfrin     |   Resolution:                                                
 Keywords:            |    Sensitive:  0                                             
----------------------+-----------------------------------------------------

Comment(by jelte):

 Responding to the cpp review part (tests are up next); since it's quite a
 big review with lots of (good!) comments, i am responding to things i want
 to explain or disagree with. Most of the comments have 'disappeared' in
 this reply; this means i agree with them and have fixed them in one of the
 commits between r2234 and the current head (right now that's r2259).

 >
 > Few of the wrapper functions have Doxygen headers.
 >

 Do any? The documentation here is going to be looked at from the python
 side, and is included in-line for that purpose. So I don't really see the
 need for doxygen in this code.

 >
 > The static_cast<>() construct is now recommended instead of the C-style
 (xxx) construct.
 >

 changes to (derived) classes, because with the way structs were used, most
 of those would have needed to be reinterpret_cast<>, as discussed on
 jabber. Updated all casts.

 >
 > There are a number of "TODO" or "XXX" comments in the code.
 >

 Fixed most of them (or removed them as they weren't relevant, for instance
 the ones that asked whether some function pointer could be NULL, the
 answer is no.)

 Left a few in, mostly for future reference if we decide to extend the
 current interface with direct iterators.


 >
 > experiments/python-binding/src/lib/dns/python/messagerenderer_python.cc
 > ===
 > The methods skip(), trim() etc. are presumably not needed because the
 buffer is available in Python and can be accessed there?  But shouldn't
 there be an entry for writeName(), which allows for compression of the
 name? (Although in that case, how does the state of the buffer get
 communicated between the MessageRenderer class and Python?)
 >

 Yeah I wasn't sure exactly how much we needed, and the only really useful
 external use I see right now is simply to create a MessageRenderer()
 object and pass it to the toWire() function of whatever you want rendered
 (mostly Message objects i figure).


 > Need to clarify whether the two elements tp_del and tp_version_tag in
 the messagerenderer_type object are correct. (Comment suggests that this
 is not clear.)
 >

 they were undocumented, but are certainly present, so I'm leaving them in
 (and removing that comment)

 >
 > experiments/python-binding/src/lib/dns/python/name_python.cc
 > ===
 >
 >
 > NameComparisonResult
 > ---
 > There is a TODO comment asking if there is also a way to just not define
 it. (because the class is only ever used to return the result of a name
 comparison?).  However, as the C++ class can be created explicitly, is
 there a case for saying that the Python equivalent can also be created
 directly? (for example, for testing purposes?).
 >

 Yeah I really didn't see this one being created manually.

 >
 > Name
 > ---


 > Name_compare: Unless it is a very old compiler, "new" can never return
 NULL when creating the NameComparisonResult object.  Should there be a
 check as to whether an exception has been raised prior to checking if the
 a value was assigned to ret->ncr?  (Same goes for creation of "Name" in
 Name_reverse.)
 >

 oh you're right, removed that check. I think we declared bad_alloc a
 special case exception where we wouldn't be checking all the time.
 Although perhaps here we should. Not sure.

 > Name_concatenate: I'm a bit uncomfortable that the catch is only for one
 of the possible exceptions that can be thrown by the "Name" constructor.
 (Although I agree that this is the only exception that can be logically
 thrown; however it does rely on the internals of the constructor itself.
 Perhaps a comment to that effect?)
 >

 Umz, i don't think the copy constructor throws anything, right (well
 except bad_alloc again). So only TooLongName can be thrown here, or am I
 missing something?

 I did change it, btw, on TooLongName it now directly throws a
 pythonexception (ie. returns NULL), and the NULLcheck after that is
 removed.

 >
 > RRType_xx: The functions RRType_<whatever> contain virtually identical
 code, most of which could be factored out into a common function.  (In
 fact, they are so close to the equivalent RRClass functions, that the
 common code is a candidate for templatisation.)
 >

 did the factoring out (also for rrclass), but the function does use a bit
 of knowledge about the types, so i didn't go all the way and make a
 template.

 >
 > experiments/python-binding/src/lib/dns/python/rdata_python.cc
 > ===
 > Most modules, when defining the s_XXX structure use a "raw" pointer to
 additional data.  However, rdata_python used a boost::shared_ptr in its
 s_Rdata structure.  Is there a reason for this?
 >

 I tried to avoid using shared_ptr as much as possible, since we already
 have a reference counting object that contains it. However, in this case I
 couldn't make it work with rrsets and messages (and the way stuff is
 added/removed there), so I opted for a shared_ptr here. Will add a comment
 saying something like this. Also this might be something to revisit if we
 take an 'api cleanup' broom through the dns cpp library (which isn't
 planned but i have some ideas for which i need a bit of time to write them
 down). (same for rrsets later)

 >
 > experiments/python-binding/src/lib/dns/python/question_python.cc
 > ===
 > Question_getXxx: inconsistent style - in an "if" statement, all other
 code puts the opening brace on the same line as the "if".
 >

 they were also unnecessary. removed.

 > Question_toWire: the OutputBuffer is declared with the explicit constant
 255 instead of the symbolic constant Name::MAX_WIRE.
 >

 MAX_WIRE + 4 :)

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