BIND 10 #2379: add python wrapper of datasrc::ZoneLoader

[BIND 10 Development]

#2379: add python wrapper of datasrc::ZoneLoader
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20121218
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  4             |                 CVSS Scoring:
         Total Hours:  10            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:9 jinmei]:

 > > Also added a check that should trigger too many DECREFS. But have not
 really thought of a good way to test reference leaks.
 > >
 > > Not sure what that last line means.
 >
 > Ah there was a typo, I meant "writing a test", and specifically
 > intended checking the object's refcount directly using
 > sys.getrefcount() like in message_python_test.py:test_get_section()
 > (for `isc.dns.Message`).
 >

 ah, ok :) In that sense we should check it for every possible scenario
 (and hence for all tests, and hence best in setUp() and tearDown().
 Removed the separate checkrefs tests again and added refcount checks to
 those (putting an optional source_client in self to check those as well).

 > > > '''zone_loader_test.py'''
 > > >
 > >
 > > None of these seem nice though... I think I dislike option 1 the
 least. Therefore I'll do that but as the last commit, so if you have a
 better idea, I can revert it.
 >
 > For this branch I think this approach is okay.  For a longer term, we
 > might want to extend `ZoneLoader` so it supports the "with" protocol.
 > Then (I think) we can write exception safe code in more concise way:
 > {{{#!python
 >     with ZoneLoader(client, name, 'foo.zone') as loader:
 >         loader.load()
 >         # if exception happens in load(), ZoneLoader.__exit__() will
 >         # ensure releasing any resource.
 > }}}
 >

 yeah I realized this over the weekend as well, and we should probably do
 that indeed, but I'm now also abusing the self.loader for the reference
 counts mentioned above :)

 >
 > Comments on the revised branch follow:
 >
 > '''pydnspp_common.cc'''
 >
 > - Mostly a pedantic point (and I myself misunderstood it until quite
 >   recently): In this case, we should actually rather use staged cast via
 >   `void*` instead of reinterpret_cast (like the original code):
 > {{{#!cpp
 >     void* p = &type;
 >     if (PyType_Ready(&type) < 0 ||
 >         PyModule_AddObject(mod, name, static_cast<PyObject*>(p)) < 0) {
 >         return (false);
 >     }
 > }}}
 >   because what we want to do is to "reinterpret" the bit sequence
 >   starting from the head address of `type` as bits of type `PyObject`.
 >   In theory, reinterpret_cast could even do more evil thing than that
 >   like returning a different address.  See also
 >   http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.HTML#658
 >

 OK changed, also shows the usefulness of doing this in its own function,
 as only a few of the originals actually did that.

 > '''pydnspp.cc'''
 >
 > - I think the idea of `initClass` makes sense.
 > - `initModulePart_Serial` can be simplified:
 > {{{#!cpp
 > initModulePart_Serial(PyObject* mod) {
 >     return (initClass(serial_type, "Serial", mod));
 > }
 > }}}
 >   same for `initModulePart_TSIG`.

 haha, i thought you might mention that, had left it as it was for some
 demented sake of consistency, but yeah, changed :)

 (also questiontype btw, which even had an incref too many, though i guess
 type reference leaks aren't as bad as normal ones)

 > - If you use `PyObjectContainer`, you'll need to except exception and
 >   do unified cleanup on failure.  Like in `initModulePart_TSIGContext`.
 >   (also see the usage description in pycppwrapper_util.h)
 >

 right, done

 > - `initModulePart_Name`: to be safer, if we fail in the initialization
 >   of `Name` we also need to release `mod` for `NameComparisonResult`.
 >

 Even cleaner, I made NameComparisonResult have its own init function, not
 sure why those were grouped tbh.


 Final commit should fix the distcheck problem.

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