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