BIND 10 trac983, updated. cca39b307de50546d7e3c4cd9fe4c2435223bf21 [trac983] allowed python representation of JSON (as well as bare string) in load()
BIND 10 source code commits
bind10-changes at lists.isc.org
Wed Jul 13 09:17:55 UTC 2011
The branch, trac983 has been updated
via cca39b307de50546d7e3c4cd9fe4c2435223bf21 (commit)
via dffeeebd09195ad602090501c8c9b05b55885596 (commit)
via 673a619cd628130b0506a5d3669fd6a4d139c790 (commit)
from f8092952b50ef238e2ffc63ccb6d17a469f22966 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit cca39b307de50546d7e3c4cd9fe4c2435223bf21
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Jul 13 02:16:51 2011 -0700
[trac983] allowed python representation of JSON (as well as bare string)
in load()
commit dffeeebd09195ad602090501c8c9b05b55885596
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Jul 13 02:16:17 2011 -0700
[trac983] added the default constructor and reset() to PyObjectContainer,
which will be used for the remaining work in trac983.
commit 673a619cd628130b0506a5d3669fd6a4d139c790
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Tue Jul 12 16:31:14 2011 -0700
[trac983] clarified comments on refcount of some python constant objects.
-----------------------------------------------------------------------
Summary of changes:
src/lib/python/isc/acl/acl.cc | 8 +-
src/lib/python/isc/acl/dns_requestloader_inc.cc | 6 +-
src/lib/python/isc/acl/dns_requestloader_python.cc | 72 +++++++++++++--
src/lib/python/isc/acl/tests/dns_test.py | 97 +++++++++++++++++---
src/lib/util/python/pycppwrapper_util.h | 29 ++++++-
5 files changed, 185 insertions(+), 27 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/python/isc/acl/acl.cc b/src/lib/python/isc/acl/acl.cc
index 0124b5a..6517a12 100644
--- a/src/lib/python/isc/acl/acl.cc
+++ b/src/lib/python/isc/acl/acl.cc
@@ -60,9 +60,11 @@ PyInit_acl(void) {
po_LoaderError = PyErr_NewException("isc.acl.LoaderError", NULL, NULL);
PyObjectContainer(po_LoaderError).installToModule(mod, "LoaderError");
- // Install module constants. Note that we can release our own
- // references to these objects because we don't have corresponding
- // C++ variables.
+ // Install module constants. Note that we can let Py_BuildValue
+ // "steal" the references to these object (by specifying false to
+ // installToModule), because, unlike the exception cases above,
+ // we don't have corresponding C++ variables (see the note in
+ // pycppwrapper_util for more details).
PyObjectContainer(Py_BuildValue("I", isc::acl::ACCEPT)).
installToModule(mod, "ACCEPT", false);
PyObjectContainer(Py_BuildValue("I", isc::acl::REJECT)).
diff --git a/src/lib/python/isc/acl/dns_requestloader_inc.cc b/src/lib/python/isc/acl/dns_requestloader_inc.cc
index 904d750..e05a580 100644
--- a/src/lib/python/isc/acl/dns_requestloader_inc.cc
+++ b/src/lib/python/isc/acl/dns_requestloader_inc.cc
@@ -65,7 +65,7 @@ this restriction may be removed.\n\
const char* const RequestLoader_load_doc = "\
load(description) -> RequestACL\n\
\n\
-Load a DNS ACL.\n\
+Load a DNS (Request) ACL.\n\
\n\
This parses an ACL list, creates internal data for each rule\n\
and returns a RequestACl object that contains all given rules.\n\
@@ -77,7 +77,9 @@ Exceptions:\n\
exception.\n\
\n\
Parameters:\n\
- description String representation of the JSON list of ACL.\n\
+ description String or Python representation of the JSON list of\n\
+ ACL. The Python representation is ones accepted by the\n\
+ standard json module.\n\
\n\
Return Value(s): The newly created RequestACL object\n\
";
diff --git a/src/lib/python/isc/acl/dns_requestloader_python.cc b/src/lib/python/isc/acl/dns_requestloader_python.cc
index a577996..aa5df67 100644
--- a/src/lib/python/isc/acl/dns_requestloader_python.cc
+++ b/src/lib/python/isc/acl/dns_requestloader_python.cc
@@ -82,13 +82,58 @@ RequestLoader_destroy(PyObject* po_self) {
Py_TYPE(self)->tp_free(self);
}
+// This helper function essentially does:
+// import json
+// return json.dumps
+// Getting access to the json module this way and call one of its functions
+// via PyObject_CallObject() may exceed the reasonably acceptable level for
+// straightforward bindings. But the alternative would be to write a Python
+// frontend for the entire module only for this conversion, which would also
+// be too much. So, right now, we implement everything within the binding
+// implementation. If future extensions require more such non trivial
+// wrappers, we should consider the frontend approach more seriously.
+PyObject*
+getJSONDumpsObj() {
+ PyObject* json_dump_obj = NULL;
+ PyObject* json_module = PyImport_AddModule("json");
+ if (json_module != NULL) {
+ PyObject* json_dict = PyModule_GetDict(json_module);
+ if (json_dict != NULL) {
+ json_dump_obj = PyDict_GetItemString(json_dict, "dumps");
+ }
+ }
+ return (json_dump_obj);
+}
+
PyObject*
RequestLoader_load(PyObject* po_self, PyObject* args) {
s_RequestLoader* const self = static_cast<s_RequestLoader*>(po_self);
- const char* acl_config;
- if (PyArg_ParseTuple(args, "s", &acl_config)) {
- try {
+ try {
+ PyObjectContainer c1, c2; // placeholder for temporary py objects
+ const char* acl_config;
+
+ // First, try string
+ int py_result = PyArg_ParseTuple(args, "s", &acl_config);
+ if (!py_result) {
+ PyErr_Clear(); // need to clear the error from ParseTuple
+
+ // If that fails, confirm the argument is a single Python object,
+ // and pass the argument to json.dumps() without conversion.
+ // Note that we should pass 'args', not 'json_obj' to
+ // PyObject_CallObject(), since this function expects a form of
+ // tuple as its argument parameter, just like ParseTuple.
+ PyObject* json_obj;
+ if (PyArg_ParseTuple(args, "O", &json_obj)) {
+ PyObject* json_dumps_obj = getJSONDumpsObj();
+ if (json_dumps_obj != NULL) {
+ c1.reset(PyObject_CallObject(json_dumps_obj, args));
+ c2.reset(Py_BuildValue("(O)", c1.get()));
+ py_result = PyArg_ParseTuple(c2.get(), "s", &acl_config);
+ }
+ }
+ }
+ if (py_result) {
shared_ptr<RequestACL> acl(
self->cppobj->load(Element::fromJSON(acl_config)));
s_RequestACL* py_acl = static_cast<s_RequestACL*>(
@@ -97,15 +142,24 @@ RequestLoader_load(PyObject* po_self, PyObject* args) {
py_acl->cppobj = acl;
}
return (py_acl);
- } catch (const exception& ex) {
- PyErr_SetString(getACLException("LoaderError"), ex.what());
- return (NULL);
- } catch (...) {
- PyErr_SetString(PyExc_SystemError, "Unexpected C++ exception");
- return (NULL);
}
+ } catch (const PyCPPWrapperException&) {
+ // If the wrapper utility throws, it's most likely because an invalid
+ // type of argument is passed (and the call to json.dumps() failed
+ // above), rather than a rare case of system errors such as memory
+ // allocation failure. So we fall through to the end of this function
+ // and raise a TypeError.
+ ;
+ } catch (const exception& ex) {
+ PyErr_SetString(getACLException("LoaderError"), ex.what());
+ return (NULL);
+ } catch (...) {
+ PyErr_SetString(PyExc_SystemError, "Unexpected C++ exception");
+ return (NULL);
}
+ PyErr_SetString(PyExc_TypeError, "RequestLoader.load() "
+ "expects str or python representation of JSON");
return (NULL);
}
diff --git a/src/lib/python/isc/acl/tests/dns_test.py b/src/lib/python/isc/acl/tests/dns_test.py
index fa6e802..acaf32b 100644
--- a/src/lib/python/isc/acl/tests/dns_test.py
+++ b/src/lib/python/isc/acl/tests/dns_test.py
@@ -32,6 +32,13 @@ def get_acl(prefix):
return REQUEST_LOADER.load('[{"action": "ACCEPT", "from": "' + \
prefix + '"}]')
+def get_acl_json(prefix):
+ '''Same as get_acl, but this function passes a Python representation of
+ JSON to the loader, not a string.'''
+ json = [{"action": "ACCEPT"}]
+ json[0]["from"] = prefix
+ return REQUEST_LOADER.load(json)
+
def get_context(address):
'''This is a simple shortcut wrapper for creating a RequestContext
object with a given IP address. Port number doesn't matter in the test
@@ -100,11 +107,14 @@ class RequestACLTest(unittest.TestCase):
def test_request_loader(self):
# these shouldn't raise an exception
REQUEST_LOADER.load('[{"action": "DROP"}]')
+ REQUEST_LOADER.load([{"action": "DROP"}])
REQUEST_LOADER.load('[{"action": "DROP", "from": "192.0.2.1"}]')
+ REQUEST_LOADER.load([{"action": "DROP", "from": "192.0.2.1"}])
- # Invalid types
- self.assertRaises(TypeError, REQUEST_LOADER.load, 1)
- self.assertRaises(TypeError, REQUEST_LOADER.load, [])
+ # Invalid types (note that arguments like '1' or '[]' is of valid
+ # 'type' (but syntax error at a higher level)). So we need to use
+ # something that is not really JSON nor string.
+ self.assertRaises(TypeError, REQUEST_LOADER.load, b'')
# Incorrect number of arguments
self.assertRaises(TypeError, REQUEST_LOADER.load,
@@ -113,66 +123,119 @@ class RequestACLTest(unittest.TestCase):
def test_bad_acl_syntax(self):
# the following are derived from loader_test.cc
self.assertRaises(LoaderError, REQUEST_LOADER.load, '{}');
+ self.assertRaises(LoaderError, REQUEST_LOADER.load, {});
self.assertRaises(LoaderError, REQUEST_LOADER.load, '42');
+ self.assertRaises(LoaderError, REQUEST_LOADER.load, 42);
self.assertRaises(LoaderError, REQUEST_LOADER.load, 'true');
+ self.assertRaises(LoaderError, REQUEST_LOADER.load, True);
self.assertRaises(LoaderError, REQUEST_LOADER.load, 'null');
+ self.assertRaises(LoaderError, REQUEST_LOADER.load, None);
self.assertRaises(LoaderError, REQUEST_LOADER.load, '"hello"');
+ self.assertRaises(LoaderError, REQUEST_LOADER.load, "hello");
self.assertRaises(LoaderError, REQUEST_LOADER.load, '[42]');
+ self.assertRaises(LoaderError, REQUEST_LOADER.load, [42]);
self.assertRaises(LoaderError, REQUEST_LOADER.load, '["hello"]');
+ self.assertRaises(LoaderError, REQUEST_LOADER.load, ["hello"]);
self.assertRaises(LoaderError, REQUEST_LOADER.load, '[[]]');
+ self.assertRaises(LoaderError, REQUEST_LOADER.load, [[]]);
self.assertRaises(LoaderError, REQUEST_LOADER.load, '[true]');
+ self.assertRaises(LoaderError, REQUEST_LOADER.load, [True]);
self.assertRaises(LoaderError, REQUEST_LOADER.load, '[null]');
+ self.assertRaises(LoaderError, REQUEST_LOADER.load, [None]);
self.assertRaises(LoaderError, REQUEST_LOADER.load, '[{}]');
+ self.assertRaises(LoaderError, REQUEST_LOADER.load, [{}]);
# the following are derived from dns_test.cc
self.assertRaises(LoaderError, REQUEST_LOADER.load,
'[{"action": "ACCEPT", "bad": "192.0.2.1"}]')
self.assertRaises(LoaderError, REQUEST_LOADER.load,
+ [{"action": "ACCEPT", "bad": "192.0.2.1"}])
+ self.assertRaises(LoaderError, REQUEST_LOADER.load,
'[{"action": "ACCEPT", "from": 4}]')
self.assertRaises(LoaderError, REQUEST_LOADER.load,
+ [{"action": "ACCEPT", "from": 4}])
+ self.assertRaises(LoaderError, REQUEST_LOADER.load,
'[{"action": "ACCEPT", "from": []}]')
self.assertRaises(LoaderError, REQUEST_LOADER.load,
+ [{"action": "ACCEPT", "from": []}])
+ self.assertRaises(LoaderError, REQUEST_LOADER.load,
'[{"action": "ACCEPT", "from": "bad"}]')
self.assertRaises(LoaderError, REQUEST_LOADER.load,
+ [{"action": "ACCEPT", "from": "bad"}])
+ self.assertRaises(LoaderError, REQUEST_LOADER.load,
'[{"action": "ACCEPT", "from": null}]')
+ self.assertRaises(LoaderError, REQUEST_LOADER.load,
+ [{"action": "ACCEPT", "from": None}])
def test_bad_acl_ipsyntax(self):
# this test is derived from ip_check_unittest.cc
self.assertRaises(LoaderError, REQUEST_LOADER.load,
'[{"action": "DROP", "from": "192.0.2.43/-1"}]')
self.assertRaises(LoaderError, REQUEST_LOADER.load,
- '[{"action": "DROP", "from": "192.0.2.43//1"')
+ [{"action": "DROP", "from": "192.0.2.43/-1"}])
+ self.assertRaises(LoaderError, REQUEST_LOADER.load,
+ '[{"action": "DROP", "from": "192.0.2.43//1"}]')
+ self.assertRaises(LoaderError, REQUEST_LOADER.load,
+ [{"action": "DROP", "from": "192.0.2.43//1"}])
+ self.assertRaises(LoaderError, REQUEST_LOADER.load,
+ '[{"action": "DROP", "from": "192.0.2.43/1/"}]')
+ self.assertRaises(LoaderError, REQUEST_LOADER.load,
+ [{"action": "DROP", "from": "192.0.2.43/1/"}])
+ self.assertRaises(LoaderError, REQUEST_LOADER.load,
+ '[{"action": "DROP", "from": "/192.0.2.43/1"}]')
self.assertRaises(LoaderError, REQUEST_LOADER.load,
- '[{"action": "DROP", "from": "192.0.2.43/1/"')
+ [{"action": "DROP", "from": "/192.0.2.43/1"}])
self.assertRaises(LoaderError, REQUEST_LOADER.load,
- '[{"action": "DROP", "from": "/192.0.2.43/1"')
+ '[{"action": "DROP", "from": "2001:db8::/xxxx"}]')
self.assertRaises(LoaderError, REQUEST_LOADER.load,
- '[{"action": "DROP", "from": "2001:db8::/xxxx"')
+ [{"action": "DROP", "from": "2001:db8::/xxxx"}])
self.assertRaises(LoaderError, REQUEST_LOADER.load,
- '[{"action": "DROP", "from": "2001:db8::/32/s"')
+ '[{"action": "DROP", "from": "2001:db8::/32/s"}]')
self.assertRaises(LoaderError, REQUEST_LOADER.load,
- '[{"action": "DROP", "from": "1/"')
+ [{"action": "DROP", "from": "2001:db8::/32/s"}])
self.assertRaises(LoaderError, REQUEST_LOADER.load,
- '[{"action": "DROP", "from": "/1"')
+ '[{"action": "DROP", "from": "1/"}]')
self.assertRaises(LoaderError, REQUEST_LOADER.load,
- '[{"action": "DROP", "from": "192.0.2.0/33"')
+ [{"action": "DROP", "from": "1/"}])
self.assertRaises(LoaderError, REQUEST_LOADER.load,
- '[{"action": "DROP", "from": "::1/129"')
+ '[{"action": "DROP", "from": "/1"}]')
+ self.assertRaises(LoaderError, REQUEST_LOADER.load,
+ [{"action": "DROP", "from": "/1"}])
+ self.assertRaises(LoaderError, REQUEST_LOADER.load,
+ '[{"action": "DROP", "from": "192.0.2.0/33"}]')
+ self.assertRaises(LoaderError, REQUEST_LOADER.load,
+ [{"action": "DROP", "from": "192.0.2.0/33"}])
+ self.assertRaises(LoaderError, REQUEST_LOADER.load,
+ '[{"action": "DROP", "from": "::1/129"}]')
+ self.assertRaises(LoaderError, REQUEST_LOADER.load,
+ [{"action": "DROP", "from": "::1/129"}])
def test_execute(self):
# tests derived from dns_test.cc. We don't directly expose checks
# in the python wrapper, so we test it via execute().
self.assertEqual(ACCEPT, get_acl('192.0.2.1').execute(CONTEXT4))
+ self.assertEqual(ACCEPT, get_acl_json('192.0.2.1').execute(CONTEXT4))
self.assertEqual(REJECT, get_acl('192.0.2.53').execute(CONTEXT4))
+ self.assertEqual(REJECT, get_acl_json('192.0.2.53').execute(CONTEXT4))
self.assertEqual(ACCEPT, get_acl('192.0.2.0/24').execute(CONTEXT4))
+ self.assertEqual(ACCEPT, get_acl_json('192.0.2.0/24').execute(CONTEXT4))
self.assertEqual(REJECT, get_acl('192.0.1.0/24').execute(CONTEXT4))
+ self.assertEqual(REJECT, get_acl_json('192.0.1.0/24').execute(CONTEXT4))
self.assertEqual(REJECT, get_acl('192.0.1.0/24').execute(CONTEXT4))
+ self.assertEqual(REJECT, get_acl_json('192.0.1.0/24').execute(CONTEXT4))
self.assertEqual(ACCEPT, get_acl('2001:db8::1').execute(CONTEXT6))
+ self.assertEqual(ACCEPT, get_acl_json('2001:db8::1').execute(CONTEXT6))
self.assertEqual(REJECT, get_acl('2001:db8::53').execute(CONTEXT6))
+ self.assertEqual(REJECT, get_acl_json('2001:db8::53').execute(CONTEXT6))
self.assertEqual(ACCEPT, get_acl('2001:db8::/64').execute(CONTEXT6))
+ self.assertEqual(ACCEPT,
+ get_acl_json('2001:db8::/64').execute(CONTEXT6))
self.assertEqual(REJECT, get_acl('2001:db8:1::/64').execute(CONTEXT6))
+ self.assertEqual(REJECT,
+ get_acl_json('2001:db8:1::/64').execute(CONTEXT6))
self.assertEqual(REJECT, get_acl('32.1.13.184').execute(CONTEXT6))
+ self.assertEqual(REJECT, get_acl_json('32.1.13.184').execute(CONTEXT6))
# A bit more complicated example, derived from resolver_config_unittest
acl = REQUEST_LOADER.load('[ {"action": "ACCEPT", ' +
@@ -187,6 +250,16 @@ class RequestACLTest(unittest.TestCase):
self.assertEqual(DROP, acl.execute(get_context('2001:db8::1')))
self.assertEqual(REJECT, acl.execute(get_context('2001:db8::2')))
+ # same test using the JSON representation
+ acl = REQUEST_LOADER.load([{"action": "ACCEPT", "from": "192.0.2.1"},
+ {"action": "REJECT",
+ "from": "192.0.2.0/24"},
+ {"action": "DROP", "from": "2001:db8::1"}])
+ self.assertEqual(ACCEPT, acl.execute(CONTEXT4))
+ self.assertEqual(REJECT, acl.execute(get_context('192.0.2.2')))
+ self.assertEqual(DROP, acl.execute(get_context('2001:db8::1')))
+ self.assertEqual(REJECT, acl.execute(get_context('2001:db8::2')))
+
def test_bad_execute(self):
acl = get_acl('192.0.2.1')
# missing parameter
diff --git a/src/lib/util/python/pycppwrapper_util.h b/src/lib/util/python/pycppwrapper_util.h
index fd55c19..3f396e2 100644
--- a/src/lib/util/python/pycppwrapper_util.h
+++ b/src/lib/util/python/pycppwrapper_util.h
@@ -94,6 +94,22 @@ public:
/// the reference to be decreased, the original bare pointer should be
/// extracted using the \c release() method.
///
+/// In some other cases, it would be convenient if it's possible to create
+/// an "empty" container and reset it with a Python object later.
+/// For example, we may want to create a temporary Python object in the
+/// middle of a function and make sure that it's valid within the rest of
+/// the function scope, while we want to make sure its reference is released
+/// when the function returns (either normally or as a result of exception).
+/// To allow this scenario, this class defines the default constructor
+/// and the \c reset() method. The default constructor allows the class
+/// object with an "empty" (NULL) Python object, while \c reset() allows
+/// the stored object to be replaced with a new one. If there's a valid
+/// object was already set, \c reset() releases its reference.
+/// In general, it's safer to construct the container object with a valid
+/// Python object pointer. The use of the default constructor and
+/// \c reset() should therefore be restricted to cases where it's
+/// absolutely necessary.
+///
/// There are two convenience methods for commonly used operations:
/// \c installAsClassVariable() to add the PyObject as a class variable
/// and \c installToModule to add the PyObject to a specified python module.
@@ -166,16 +182,27 @@ public:
/// exception in a python biding written in C/C++. See the code comment
/// of the method for more details.
struct PyObjectContainer {
+ PyObjectContainer() : obj_(NULL) {}
PyObjectContainer(PyObject* obj) : obj_(obj) {
if (obj_ == NULL) {
isc_throw(PyCPPWrapperException, "Unexpected NULL PyObject, "
"probably due to short memory");
}
}
- virtual ~PyObjectContainer() {
+ ~PyObjectContainer() {
+ if (obj_ != NULL) {
+ Py_DECREF(obj_);
+ }
+ }
+ void reset(PyObject* obj) {
+ if (obj == NULL) {
+ isc_throw(PyCPPWrapperException, "Unexpected NULL PyObject, "
+ "probably due to short memory");
+ }
if (obj_ != NULL) {
Py_DECREF(obj_);
}
+ obj_ = obj;
}
PyObject* get() {
return (obj_);
More information about the bind10-changes
mailing list