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:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''pydnspp.cc'''

 I'd like to make it a bit more safer at this opportunity using
 `PyObjectContainer` and `installToModule()`.  See, e.g.
 src/lib/python/isc/util/cio/socketsession_python.cc

 '''client_python.h'''

 - I'd like to see some documentation for
 `PyDataSourceClient_ToDataSourceClient()`.

 '''client_python.cc'''

 - the what() message of the exception seems incorrect (it's not `Name`):
 {{{#!cpp
     if (client_obj == NULL) {
         isc_throw(PyCPPWrapperException,
                   "obj argument NULL in Name PyObject conversion");
     }
 }}}
 - not a big deal in this context, but I suspect technically `client`
   cannot be a const pointer, because we are effectively going to allow
   this object to be modified.  So, this matches the reality better:
 {{{#!cpp
     s_DataSourceClient* client =
 static_cast<s_DataSourceClient*>(client_obj);
 }}}

 '''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.
 - With this code, the error string may not always be helpful:
 {{{#!cpp
     if (!PyArg_ParseTuple(args, "O!O!s", &datasourceclient_type,
                           &po_target_client, &name_type, &po_name,
                           &master_file) &&
         !PyArg_ParseTuple(args, "O!O!O!", &datasourceclient_type,
                           &po_target_client, &name_type, &po_name,
                           &datasourceclient_type, &po_source_client)
        ) {
         return (-1);
     }
 }}}
   e.g., consider the case of
 {{{#!python
     isc.datasrc.ZoneLoader(client, name, 1)
 }}}
   It would say "TypeError: must be str, not int", but it may not
   necessarily have to be str (can be another client).  So in such
   cases I'd add our own error string, saying some error in parameters
   and what are acceptable.
 - is it okay not to gain a reference to source_client?
 - `ZoneLoader_loadIncremental`: maybe a matter of taste, but we don't
   need `complete`.

 '''zone_loader_test.py'''

 - some resources don't seem to be cleaned up cleanly in case a test
   fails.  for example, if we do this in test_bad_file
 {{{#!python
         self.check_zone_soa(ORIG_SOA_TXT)
         loader = isc.datasrc.ZoneLoader(self.client, self.test_name,
                                         'no such file')
         loader.load()
         ...
 }}}
   some other tests also fail with "database is locked".
 - not looking into the code, but does this case "no such zone in
   source"?
 {{{#!python
     def test_no_such_zone_in_source(self):
         source_client = isc.datasrc.DataSourceClient('sqlite3',
 DB_SOURCE_CLIENT_CONFIG)
         self.assertRaises(isc.datasrc.Error, isc.datasrc.ZoneLoader,
                           self.client, isc.dns.Name("unknownzone"),
                           source_client)
 }}}
   maybe it happens to be so, but to be sure it's better to use an
   existing zone for the destination but not in the source (or do we do
   this?  If so, please clarify that in the comment).
 - the tests seem to be pretty different from the C++ counterpart.
   does the Python version have the same level of coverage?

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

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