Opened 10 years ago

Closed 9 years ago

#181 closed task (fixed)

python wrappers to support xfr

Reported by: larissas Owned by: jelte
Priority: medium Milestone: 05. 3rd Incremental Release: Serious Secondary
Component: xfrin Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

(task 123)

2 week work estimate. no further details in notes :(

Subtickets

Attachments (2)

CppReview.txt (8.6 KB) - added by stephen 9 years ago.
Review of the C++ code for revision 2111
PythonTestReview.txt (7.7 KB) - added by stephen 9 years ago.
Review of the Python Tests for Revision 2111

Download all attachments as: .zip

Change History (18)

comment:1 Changed 9 years ago by shane

  • Milestone changed from 04. 2nd Incremental Release: Early Adopters to 05. 3rd Incremental Release

comment:2 Changed 9 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from new to reviewing

As this was more or less a continuation of the initial work done by Francis, this ticket has its branch in experiments/python-binding.

The for python is done insofar that most of the functionality is wrapped, and at least everything needed currently in python is. Since it is also a moving target, there is a point where we have to say it is done for now and i it needs updates they'll have to go into their own tickets.

The code lives in src/lib/dns/python, and there is also a README to get you started. Updates to existing python scripts were also made as necessary (class naming and capitalization changed compared to the boost.python version).

I've synced the branch with trunk, to get rid of the conflicts due to lots of updates there (and this branch getting progressively out of sync).

I'm not sure if this should be one big review or a few separate ones, it's quite a big change...

comment:3 follow-up: Changed 9 years ago by jelte

Oh I forgot to mention, the easiest way to get a full diff (since there were a few trunk syncs in there) is probably a reintegration merge to trunk (without committing it);

~/svn/bind10/trunk> svn merge --reintegrade svn+ssh://bind10.isc.org/svn/bind10/experiments/python-binding
~/svn/bind10/trunk> svn diff

comment:4 in reply to: ↑ 3 Changed 9 years ago by jelte

Replying to jelte:

Oh I forgot to mention, the easiest way to get a full diff (since there were a few trunk syncs in there) is probably a reintegration merge to trunk (without committing it);

~/svn/bind10/trunk> svn merge --reintegrade svn+ssh://bind10.isc.org/svn/bind10/experiments/python-binding

--reintegrate
that is. Reintegrade sounds like a sports drink.

comment:5 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to stephen

Changed 9 years ago by stephen

Review of the C++ code for revision 2111

Changed 9 years ago by stephen

Review of the Python Tests for Revision 2111

comment:6 Changed 9 years ago by stephen

Checked differences thrown up by the "svn merge --reintegrate" command above - all seems OK.

comment:7 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

comment:8 Changed 9 years ago 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 :)

comment:9 Changed 9 years ago by jelte

The tests review has been handled in revision r2264

I had nothing else to add to them, good review, thanks.

comment:10 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

...and while this wasn't merged yet i also parenthesized all return values, and removed some now unnecessary static_cast statements in r2265

Thanks for the review, giving the ticket back so you can check if your comments were handled to your satisfaction. If so, assign back to me again and i'll merge.

comment:11 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

Just a few minor things missed (listed below), but otherwise all done; thank you. I have a couple of remarks that may be relevant either as a further ticket or for general discussion so I will mention them here so they are recorded somewhere.

experiments/python-binding/src/lib/dns/python/libdns_python_common.cc
Remark: the associated header file is included using angle brackets, not quotes. Strictly speaking the only difference between the two is that the '#include <file>' form searches for the file in the standard list of system directories, whereas the '#include "file"' form causes the directory holding the file being compiled to be searched as well. However, as the list of include directories is extended by the "-I" switch on the command line, either form works. My preference - and it is just a personal one - is to use the '<>' form to indicate that the file is an operating system-supplied one and the '""' form for everything else.

experiments/python-binding/src/lib/dns/python/name_python.cc

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?

Remark: No you are not - I'm just being overly cautious. My point was that the constructor can throw a number of exceptions but only one is caught here. We are relying on a knowledge of constructor internals and on the knowledge of the arguments passed to it to justify not catching the others. A change of implementation could lead to other exceptions being thrown in the future. So future-proofing the code would involve:

   try {
      ...
   } catch (isc::dns::TooLongName tln) {
      ...
   } catch (isc::Exception ise) {
      // Any other programmer-defined exception; return an unexpected exception message
   }

But this is a general point regarding defensive programming that should be discussed elsewhere.

experiments/python-binding/src/lib/dns/python/name_python.cc

initModulePart_Name: When adding the constants to the module, can't the
numeric values in the calls to Py_BuildValue be accessed symbolically via
a call to Name::<constant_name>?

This has been done for the NameComparisonResult constants, but not for the constants such as "MAX_WIRE" in the Py_BuildValue calls embedded in calls to addClassVariable.

experiments/python-binding/src/lib/dns/python/rrttl.cc
There is a C-style cast in RRTTL_toWire().

experiments/python-binding/src/lib/dns/python/tests/message_python_test.py
Still a direct call to the __str__() method (instead of using the str() function).

experiments/python-binding/src/lib/dns/python/tests/rrtype_python_test.py
Still a direct call to the __str__() method (instead of using the str() function).

comment:12 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

Ok, done in r2323 (found a few more c-style casts and replaced them too).

comment:13 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

I've checked the changes (in revision 2340). All are OK, but I have one last question: in name_python.cc (line 647)

root_name->name = new Name(".")

was replaced with

root_name->name = const_cast<Name*>(&Name::ROOT_NAME())

In the original assignment, the "Name" object was allocated on the heap. In the replacement, the "Name" object returned by Name::ROOT_NAME() is a statically-declared singleton. If root_name is ever destroyed, the destructor will not be able to deleted the pointed-to Name object.

comment:14 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

Ok, see r2342, now uses a Name object allocated with new.

comment:15 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

Happy with that - you can go ahead and merge the code.

comment:16 Changed 9 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged in r2361, closing ticket.

Note: See TracTickets for help on using tickets.