BIND 10 #2853: Python wrapper of data source extensions

[BIND 10 Development]

#2853: Python wrapper of data source extensions
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  vorner
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130625
         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
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => vorner


Comment:

 Hi Jinmei

 Replying to [comment:24 jinmei]:
 > Replying to [comment:22 muks]:
 >
 > > Replying to [comment:21 jinmei]:
 > > > This seems to be problematic:
 > > >
 > > > {{{#!cpp
 > > >             PyObject *tup = Py_BuildValue("(ssI)",
 > > >
 status[i].getName().c_str(),
 > > >
 status[i].getSegmentType().c_str(),
 > > >
 status[i].getSegmentState());
 > > > }}}
 > > >
 > > > because if the state is unused, getSegmentType() throws.
 > >
 > > I have added code to handle this exception, and also updated the C++
 > > `getStatus()` implementation to be more correct (#2943 should be a
 > > duplicate now).  However, from the Python code I can't find a way to
 > > trigger that exception to test it.
 >
 > Regarding the test: if you meant how to test the case of unused state,
 > you should simply use a data source with disabling in-memory cache.

 With `MasterFiles`, it expects the cache to be always enabled. With
 other names, it looks for a loadable `.so` module of that name.

 > Regarding the code, I personally think it's better to check the state
 > explicitly and use Py_None if it's SEGMENT_UNUSED.

 I wanted to avoid doing this in the Python wrapper, because we are
 introducing this `SEGMENT_UNUSED` checking behavior in two places (one
 in the C++ code and another in the Python wrapper). Is there something
 wrong with how we do it in the current Python wrapper?

 > And, not directly on this point, but on a closer look the code looks
 > generally unsafe.  For example, in `ConfigurableClientList_getStatus`
 > if this fails:
 >
 > {{{#!cpp
 >             PyObject* tup = Py_BuildValue("(sOI)",
 >                                           status[i].getName().c_str(),
 >                                           segment_type,
 >                                           status[i].getSegmentState());
 > }}}
 >
 > then the reference to segment_type will be lost.  You'll need a help
 > of `PyObjectContainer` here.

 By "fails", do you mean it throws an exception that causes
 control flow to exit that block at that location? `Py_BuildValue()` is a
 C function. If there is a failure within it, it will return `NULL`. In
 this case, we first drop our reference on `segment_type` before
 returning a failure. `status[i]`, `getName()` and `getSegmentState()`
 should not throw in the way our code is written.

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