BIND 10 #2908: Python wrapper for ZoneTableAccessor and getZoneTableAccessor

[BIND 10 Development]

#2908: Python wrapper for ZoneTableAccessor and getZoneTableAccessor
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  pselkirk
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130611
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by pselkirk):

 Replying to [comment:8 vorner]:

 > '''Potential problems'''
 >
 > Is it OK to delete the accessor before the client list it comes from? If
 it should not happen, we should ensure it with the `base_obj` trick like
 in the case of the iterator.

 It's okay to delete the accessor before the client list. It's not okay to
 delete the client list before the accessor, which appears to be the intent
 of the `base_obj` trick. So I added it to `createZoneTableAccessorObject`.

 > In `ZoneTableIterator_next` (unless you agree to delete it), the catch-
 all omits setting the error string.

 Deleted it.

 > You should be returning by py_ReturnTrue and pyReturFalse (not sure
 about the exact spelling) here, not by integer. Python has booleans.
 > {{{#!c++
 > PyObject*
 > ZoneTableIterator_isLast(PyObject* po_self, PyObject*) {
 >     s_ZoneTableIterator* self =
 static_cast<s_ZoneTableIterator*>(po_self);
 >     try {
 >         return (Py_BuildValue("i", self->cppobj->isLast()));
 >     } catch (...) {
 >         return (NULL);
 >     }
 > }}}

 Deleted that as well.

 > There should be a wrapper for isc::dns::Name, returning string is wrong.

 Okay, found `isc::dns::python::createNameObject()`

 > '''Style'''
 >
 > Things like this look awkward:
 > {{{#!c++
 > accessor.reset(Py_BuildValue(""));
 > }}}
 >
 > Should it be just py_ReturnNone?

 I borrowed that from `ConfigurableClientList_find`, but that returns three
 objects, while `ConfigurableClientList_getZoneTableAccessor` returns one,
 so I simplified out the `PyObjectContainer`.

 > If the python methods are named `get_zone_table` and `get_zones`, the
 C++ wrapper functions should be renamed accordingly.

 I renamed the python methods to match the C++ methods.

 > The indentation looks wrong here. Is it a tab?:
 >
 > {{{
 > +    def test_get_zone_table(self):
 > +        """
 > +       Test that we can get the zone table accessor and, thereby,
 > +        the zone table iterator.
 > +        """
 > }}}

 No, just a fat finger.

 > The commented out includes should probably be removed.

 I meant to do that, just forgot at 3am.

 > The reset here is because it needed to be deleted before the base
 object. If there is no base object (and unless you need to return it, as
 mentioned above), it is useless:
 >
 > {{{#!c++
 > void
 > ZoneTableAccessor_destroy(PyObject* po_self) {
 >     s_ZoneTableAccessor* const self =
 >         static_cast<s_ZoneTableAccessor*>(po_self);
 >     // cppobj is a shared ptr, but to make sure things are not destroyed
 in
 >     // the wrong order, we reset it here.
 >     self->cppobj.reset();
 >     Py_TYPE(self)->tp_free(self);
 > }}}

 There is now a base object chain from iterator to accessor to client list,
 so I think this can stay.

 > '''Documentation'''
 >
 > This still talks about accessors:

 Renamed zone_table back to accessor, so this can stand.

 > This includes `base_obj` in the description, but the parameter is not
 there:

 Fixed the implementation (and the prototype).

 >
 > This talks about C++, while the documentation is for python:
 >
 > {{{#!c++
 > const char* const ZoneTableIterator_doc = "\
 > Read-only iterator to a zone table.\n\
 > \n\
 > You can get an instance of the ZoneTableIterator from the\
 > ZoneTableAccessor.get_iterator() method. The actual concrete\
 > C++ implementation will be different depending on the actual data
 source\
 > used. This is the abstract interface.\n\
 > }}}

 Ah, that was a direct cut-and-paste from `ZoneIterator_doc`, which passed
 review a couple years ago.

 Still, I take your point. For now, I've just removed the mention of C++.

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