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

BIND 10 Development do-not-reply at isc.org
Fri Dec 14 19:12:59 UTC 2012


#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
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:8 jelte]:

 > > '''zone_loader_python.cc'''
 > >
 > > - `ZoneLoader_init`: it doesn't seem to be exception safe regarding
 > >   the reference to po_target_client.  I suspect it should be protected
 > >   using `PyObjectContainer`.  Also writing in that case (that checks
 > >   the refcount of the target client) is a good idea.
 >
 > Done, though the incref/set could probably just go at the end in this
 specific scenario. But I may be misunderstanding the use of the container
 here :)

 Ah, right, I overlooked it.  In this specific case it should suffice
 to just defer `Py_INCREF`.  I'd leave it to you whether to simplify
 the code that way.

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

 > > '''zone_loader_test.py'''
 > >
 > > - some resources don't seem to be cleaned up cleanly in case a test
 > >   fails.  [...]
 >
 > Hmz, initially I thought this may be a refcounting problem (and it looks
 like it is, in a way, but not what I was expecting).
 >
 > Turns out, when there is an exception, local variables aren't actually
 completely destroyed until all tests finish (exception context gets a
 reference too). Normally, this isn't much of a problem, but in the case of
 the loader, we can only have 1 instance at a time (since it locks the db).

 Yeah I noticed that in #2380, too.

 > But it certainly messes up normal C++-style RAII usage...
 >
 > There are several workarounds;
 >
 > - put the loader as a member in the test class, and set it to None in
 tearDown(); (tbh, i was surprised that that worked)
 > [...]
 >
 > 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.
 }}}

 > > '''zone_loader_inc.cc'''
 > >
 > > The same comment as that for #2542 applies.  I'm attaching the C++
 > > file that the auto-generation tool produces for reference (if we want
 > > to use it we still normally need to edit it a bit).
 >
 > Updated it a bit and replaced the original.
 >
 > Two comments if you're cleaning up your script:
 > - 'void' tends to be None in python (as a return value for methods)
 > - the \note for loadIncremental was omitted

 Hmm, okay.  The script should be able to handle "\note"s, but this is
 probably an exceptional case.

 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

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

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

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


More information about the bind10-tickets mailing list