From: Paul Selkirk Date: Thu, 6 Jun 2013 02:30:36 +0000 (-0400) Subject: [2908] numerous fixes from review X-Git-Tag: bind10-1.2.0beta1-release~411^2~4 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=003aa4bedf13cd9d7b2e73983aa593deb6281435;p=thirdparty%2Fkea.git [2908] numerous fixes from review - remove ZoneTableIterator.next, is_last - rename get_zone_table -> get_zone_table_accessor - rename get_zones -> get_iterator - allow get_zone_table_accessor(None) instead of "" for "any data source" - add s_ZoneTableAccessor.base_obj to ensure in-order destruction - get_current returns Name instead of string - additional test cases for iterator - general code cleanup and doc cleanup --- diff --git a/src/lib/python/isc/datasrc/configurableclientlist_python.cc b/src/lib/python/isc/datasrc/configurableclientlist_python.cc index 87cffba214..a1d3ab4453 100644 --- a/src/lib/python/isc/datasrc/configurableclientlist_python.cc +++ b/src/lib/python/isc/datasrc/configurableclientlist_python.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any // purpose with or without fee is hereby granted, provided that the above @@ -178,16 +178,17 @@ ConfigurableClientList_getZoneTableAccessor(PyObject* po_self, PyObject* args) { try { const char* datasrc_name; int use_cache; - if (PyArg_ParseTuple(args, "si", &datasrc_name, &use_cache)) { + if (PyArg_ParseTuple(args, "zi", &datasrc_name, &use_cache)) { + // python 'None' will be read as NULL, which we convert to an + // empty string, meaning "any data source" + const std::string name(datasrc_name ? datasrc_name : ""); const ConstZoneTableAccessorPtr - z(self->cppobj->getZoneTableAccessor(datasrc_name, use_cache)); - PyObjectContainer accessor; + z(self->cppobj->getZoneTableAccessor(name, use_cache)); if (z == NULL) { - accessor.reset(Py_BuildValue("")); + Py_RETURN_NONE; } else { - accessor.reset(createZoneTableAccessorObject(z)); + return (createZoneTableAccessorObject(z, po_self)); } - return (Py_BuildValue("O", accessor.get())); } else { return (NULL); } @@ -243,9 +244,10 @@ you don't need it, but if you do need it, it is better to set it to True\ instead of getting it from the datasrc_client later.\n\ \n\ If no answer is found, the datasrc_client and zone_finder are None." }, - { "get_zone_table", ConfigurableClientList_getZoneTableAccessor, + { "get_zone_table_accessor", ConfigurableClientList_getZoneTableAccessor, METH_VARARGS, -"get_zone_table(datasrc_name, use_cache) -> isc.datasrc.ZoneTableAccessor\n\ +"get_zone_table_accessor(datasrc_name, use_cache) -> \ +isc.datasrc.ZoneTableAccessor\n\ \n\ Create a ZoneTableAccessor object for the specified data source.\n\ \n\ diff --git a/src/lib/python/isc/datasrc/tests/clientlist_test.py b/src/lib/python/isc/datasrc/tests/clientlist_test.py index 68d923d71c..6ade4e4eb6 100644 --- a/src/lib/python/isc/datasrc/tests/clientlist_test.py +++ b/src/lib/python/isc/datasrc/tests/clientlist_test.py @@ -151,16 +151,16 @@ class ClientListTest(unittest.TestCase): self.assertRaises(TypeError, self.clist.find, "example.org") self.assertRaises(TypeError, self.clist.find) - def test_get_zone_table(self): + def test_get_zone_table_accessor(self): """ - Test that we can get the zone table accessor and, thereby, + Test that we can get the zone table accessor and, thereby, the zone table iterator. """ self.clist = isc.datasrc.ConfigurableClientList(isc.dns.RRClass.IN) # null configuration self.clist.configure("[]", True) - self.assertIsNone(self.clist.get_zone_table("", True)) + self.assertIsNone(self.clist.get_zone_table_accessor(None, True)) # empty configuration self.clist.configure('''[{ @@ -169,15 +169,13 @@ class ClientListTest(unittest.TestCase): "cache-enable": true }]''', True) # bogus datasrc - self.assertIsNone(self.clist.get_zone_table("bogus", True)) + self.assertIsNone(self.clist.get_zone_table_accessor("bogus", True)) # first datasrc - empty zone table - table = self.clist.get_zone_table("", True) + table = self.clist.get_zone_table_accessor(None, True) self.assertIsNotNone(table) - zones = table.get_zones() - self.assertIsNotNone(zones) - self.assertTrue(zones.is_last()) - self.assertRaises(isc.datasrc.Error, zones.get_current) - self.assertRaises(isc.datasrc.Error, zones.next) + iterator = table.get_iterator() + self.assertIsNotNone(iterator) + self.assertRaises(isc.datasrc.Error, iterator.get_current) # normal configuration self.clist.configure('''[{ @@ -187,31 +185,27 @@ class ClientListTest(unittest.TestCase): }, "cache-enable": true }]''', True) - # use_cache = false + # !use_cache => NotImplemented self.assertRaises(isc.datasrc.Error, - self.clist.get_zone_table, "", False) + self.clist.get_zone_table_accessor, None, False) # bogus datasrc - self.assertIsNone(self.clist.get_zone_table("bogus", True)) + self.assertIsNone(self.clist.get_zone_table_accessor("bogus", True)) # first datasrc - table = self.clist.get_zone_table("", True) + table = self.clist.get_zone_table_accessor(None, True) self.assertIsNotNone(table) - zones = table.get_zones() - self.assertIsNotNone(zones) - self.assertFalse(zones.is_last()) - index, origin = zones.get_current() - self.assertEqual(origin, "example.org.") - zones.next() - self.assertTrue(zones.is_last()) - # reset iterator and count zones - zones = table.get_zones() - self.assertEqual(1, len(list(zones))) + iterator = table.get_iterator() + self.assertIsNotNone(iterator) + index, origin = iterator.get_current() + self.assertEqual(origin.to_text(), "example.org.") + self.assertEqual(1, len(list(iterator))) # named datasrc - zones = self.clist.get_zone_table("MasterFiles", True).get_zones() - index, origin = zones.get_current() - self.assertEqual(origin, "example.org.") - self.assertEqual(1, len(list(zones))) + table = self.clist.get_zone_table_accessor("MasterFiles", True) + iterator = table.get_iterator() + index, origin = iterator.get_current() + self.assertEqual(origin.to_text(), "example.org.") + self.assertEqual(1, len(list(iterator))) # longer zone list for non-trivial iteration self.clist.configure('''[{ @@ -225,9 +219,23 @@ class ClientListTest(unittest.TestCase): }, "cache-enable": true }]''', True) - zonelist = list(self.clist.get_zone_table("", True).get_zones()) + zonelist = list(self.clist.get_zone_table_accessor(None, True). + get_iterator()) self.assertEqual(5, len(zonelist)) - self.assertTrue((0, "example.net.") in zonelist) + self.assertTrue((0, isc.dns.Name("example.net.")) in zonelist) + + # ensure the iterator returns exactly and only the zones we expect + zonelist = [ + isc.dns.Name("example.org"), + isc.dns.Name("example.com"), + isc.dns.Name("example.net"), + isc.dns.Name("example.biz"), + isc.dns.Name("example.edu")] + table = self.clist.get_zone_table_accessor("MasterFiles", True) + for index, zone in table.get_iterator(): + self.assertTrue(zone in zonelist) + zonelist.remove(zone) + self.assertEqual(0, len(zonelist)) if __name__ == "__main__": isc.log.init("bind10") diff --git a/src/lib/python/isc/datasrc/zonetable_accessor_python.cc b/src/lib/python/isc/datasrc/zonetable_accessor_python.cc index de5884274e..2919a0f0c9 100644 --- a/src/lib/python/isc/datasrc/zonetable_accessor_python.cc +++ b/src/lib/python/isc/datasrc/zonetable_accessor_python.cc @@ -20,8 +20,6 @@ // http://docs.python.org/py3k/extending/extending.html#a-simple-example #include -//#include -//#include #include #include "datasrc.h" @@ -38,8 +36,36 @@ class s_ZoneTableAccessor : public PyObject { public: s_ZoneTableAccessor() : cppobj(ConstZoneTableAccessorPtr()) {}; ConstZoneTableAccessorPtr cppobj; + // This is a reference to a base object; if the object of this class + // depends on another object to be in scope during its lifetime, + // we use INCREF the base object upon creation, and DECREF it at + // the end of the destructor + // This is an optional argument to createXXX(). If NULL, it is ignored. + PyObject* base_obj; }; +int +ZoneTableAccessor_init(PyObject*, PyObject*, PyObject*) { + // can't be called directly + PyErr_SetString(PyExc_TypeError, + "ZoneTableAccessor cannot be constructed directly"); + + return (-1); +} + +void +ZoneTableAccessor_destroy(PyObject* po_self) { + s_ZoneTableAccessor* const self = + static_cast(po_self); + // cppobj is a shared ptr, but to make sure things are not destroyed in + // the wrong order, we reset it here. + self->cppobj.reset(); + if (self->base_obj != NULL) { + Py_DECREF(self->base_obj); + } + Py_TYPE(self)->tp_free(self); +} + PyObject* ZoneTableAccessor_getIterator(PyObject* po_self, PyObject* args) { s_ZoneTableAccessor* const self = @@ -64,9 +90,9 @@ ZoneTableAccessor_getIterator(PyObject* po_self, PyObject* args) { // 3. Argument type // 4. Documentation PyMethodDef ZoneTableAccessor_methods[] = { - { "get_zones", + { "get_iterator", ZoneTableAccessor_getIterator, METH_NOARGS, -"get_zones() -> isc.datasrc.ZoneTableIterator\n\ +"get_iterator() -> isc.datasrc.ZoneTableIterator\n\ \n\ Return a zone table iterator.\n\ \n" }, @@ -79,25 +105,6 @@ An accessor to a zone table for a data source.\n\ This class object is intended to be used by applications that load zones\ into memory, so that the application can get a list of zones to be loaded."; -int -ZoneTableAccessor_init(PyObject*, PyObject*, PyObject*) { - // can't be called directly - PyErr_SetString(PyExc_TypeError, - "ZoneTableAccessor cannot be constructed directly"); - - return (-1); -} - -void -ZoneTableAccessor_destroy(PyObject* po_self) { - s_ZoneTableAccessor* const self = - static_cast(po_self); - // cppobj is a shared ptr, but to make sure things are not destroyed in - // the wrong order, we reset it here. - self->cppobj.reset(); - Py_TYPE(self)->tp_free(self); -} - } // end anonymous namespace namespace isc { @@ -157,11 +164,17 @@ PyTypeObject zonetableaccessor_type = { }; PyObject* -createZoneTableAccessorObject(isc::datasrc::ConstZoneTableAccessorPtr source) { +createZoneTableAccessorObject(isc::datasrc::ConstZoneTableAccessorPtr source, + PyObject* base_obj) +{ s_ZoneTableAccessor* py_zt = static_cast( zonetableaccessor_type.tp_alloc(&zonetableaccessor_type, 0)); if (py_zt != NULL) { py_zt->cppobj = source; + py_zt->base_obj = base_obj; + if (base_obj != NULL) { + Py_INCREF(base_obj); + } } return (py_zt); } diff --git a/src/lib/python/isc/datasrc/zonetable_accessor_python.h b/src/lib/python/isc/datasrc/zonetable_accessor_python.h index 00d5bb1f1f..6ebcd9218e 100644 --- a/src/lib/python/isc/datasrc/zonetable_accessor_python.h +++ b/src/lib/python/isc/datasrc/zonetable_accessor_python.h @@ -31,7 +31,7 @@ extern PyTypeObject zonetableaccessor_type; /// this zone iterator is destroyed, making sure that the /// base object is never destroyed before this zonefinder. PyObject* createZoneTableAccessorObject( - isc::datasrc::ConstZoneTableAccessorPtr source); + isc::datasrc::ConstZoneTableAccessorPtr source, PyObject* base_obj); } // namespace python } // namespace datasrc diff --git a/src/lib/python/isc/datasrc/zonetable_iterator_python.cc b/src/lib/python/isc/datasrc/zonetable_iterator_python.cc index 46e4578b76..a67e5cf6e7 100644 --- a/src/lib/python/isc/datasrc/zonetable_iterator_python.cc +++ b/src/lib/python/isc/datasrc/zonetable_iterator_python.cc @@ -20,8 +20,8 @@ // http://docs.python.org/py3k/extending/extending.html#a-simple-example #include -//#include #include +#include #include "datasrc.h" #include "zonetable_iterator_python.h" @@ -72,39 +72,13 @@ ZoneTableIterator_destroy(s_ZoneTableIterator* const self) { // We declare the functions here, the definitions are below // the type definition of the object, since both can use the other // -PyObject* -ZoneTableIterator_isLast(PyObject* po_self, PyObject*) { - s_ZoneTableIterator* self = static_cast(po_self); - try { - return (Py_BuildValue("i", self->cppobj->isLast())); - } catch (...) { - return (NULL); - } -} - -PyObject* -ZoneTableIterator_next(PyObject* po_self, PyObject*) { - s_ZoneTableIterator* self = static_cast(po_self); - try { - self->cppobj->next(); - return (Py_BuildValue("")); - } catch (const std::exception& ex) { - // isc::InvalidOperation is thrown when we call next() - // when we are already done iterating ('iterating past end') - // We could also simply return None again - PyErr_SetString(getDataSourceException("Error"), ex.what()); - return (NULL); - } catch (...) { - return (NULL); - } -} - PyObject* ZoneTableIterator_getCurrent(PyObject* po_self, PyObject*) { s_ZoneTableIterator* self = static_cast(po_self); try { const isc::datasrc::ZoneSpec& zs = self->cppobj->getCurrent(); - return (Py_BuildValue("is", zs.index, zs.origin.toText().c_str())); + return (Py_BuildValue("iO", zs.index, + isc::dns::python::createNameObject(zs.origin))); } catch (const std::exception& ex) { // isc::InvalidOperation is thrown when we call getCurrent() // when we are already done iterating ('iterating past end') @@ -119,13 +93,13 @@ ZoneTableIterator_getCurrent(PyObject* po_self, PyObject*) { } PyObject* -ZoneTableIterator_piter(PyObject *self) { +ZoneTableIterator_iter(PyObject *self) { Py_INCREF(self); return (self); } PyObject* -ZoneTableIterator_pnext(PyObject* po_self) { +ZoneTableIterator_next(PyObject* po_self) { s_ZoneTableIterator* self = static_cast(po_self); if (!self->cppobj || self->cppobj->isLast()) { return (NULL); @@ -145,14 +119,6 @@ ZoneTableIterator_pnext(PyObject* po_self) { } PyMethodDef ZoneTableIterator_methods[] = { - { "is_last", ZoneTableIterator_isLast, METH_NOARGS, -"is_last() -> bool\n\ -\n\ -Return whether the iterator is at the end of the zone table.\n" }, - { "next", ZoneTableIterator_next, METH_NOARGS, -"next()\n\ -\n\ -Move the iterator to the next zone of the table.\n" }, { "get_current", ZoneTableIterator_getCurrent, METH_NOARGS, "get_current() -> isc.datasrc.ZoneSpec\n\ \n\ @@ -168,9 +134,7 @@ const char* const ZoneTableIterator_doc = "\ Read-only iterator to a zone table.\n\ \n\ You can get an instance of the ZoneTableIterator from the\ -ZoneTableAccessor.get_iterator() method. The actual concrete\ -C++ implementation will be different depending on the actual data source\ -used. This is the abstract interface.\n\ +ZoneTableAccessor.get_iterator() method.\n\ \n\ There's no way to start iterating from the beginning again or return.\n\ \n\ @@ -209,8 +173,8 @@ PyTypeObject zonetableiterator_type = { NULL, // tp_clear NULL, // tp_richcompare 0, // tp_weaklistoffset - ZoneTableIterator_piter, // tp_iter - ZoneTableIterator_pnext, // tp_iternext + ZoneTableIterator_iter, // tp_iter + ZoneTableIterator_next, // tp_iternext ZoneTableIterator_methods, // tp_methods NULL, // tp_members NULL, // tp_getset