From b5fba1480009998e2aaa3a58b4427e0cc540b62b Mon Sep 17 00:00:00 2001 From: Martin Thomas Date: Thu, 9 Jan 2014 10:59:57 -0600 Subject: [PATCH 1/4] Added error handling and reporting for python module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added default values for named parameters so that we aren’t given uninitialized junk. Added simple parameter checking (capacity must be 1 or more, error_range must be less than 1) Added new exception type to be used when reporting errors Added check for existing filter in dealloc. --- pydablooms/pydablooms.c | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/pydablooms/pydablooms.c b/pydablooms/pydablooms.c index 004c341..167112a 100644 --- a/pydablooms/pydablooms.c +++ b/pydablooms/pydablooms.c @@ -4,6 +4,8 @@ int Py_ModuleVersion = 1; +static PyObject *DabloomsError; + typedef struct { PyObject_HEAD scaling_bloom_t *filter; /* Type-specific fields go here. */ @@ -11,10 +13,13 @@ typedef struct { static void Dablooms_dealloc(Dablooms *self) { - free_scaling_bloom(self->filter); + if(self->filter) + free_scaling_bloom(self->filter); self->ob_type->tp_free((PyObject *)self); } +static int Dablooms_init(Dablooms *self, PyObject *args, PyObject *kwds); + static PyObject *Dablooms_new(PyTypeObject *type, PyObject *args, PyObject *kwds) { Dablooms *self; @@ -24,21 +29,39 @@ static PyObject *Dablooms_new(PyTypeObject *type, PyObject *args, PyObject *kwds } self->filter = NULL; - return (PyObject *) self; +// if(! Dablooms_init(self, args, kwds)) +// return (PyObject *) self; +// else { +// PyErr_SetString(PyExc_ZeroDivisionError, "Bloom creation failed"); +// return NULL; +// } } static int Dablooms_init(Dablooms *self, PyObject *args, PyObject *kwds) { - double error_rate; - const char *filepath; - unsigned int capacity; + double error_rate = 0.1; + const char *filepath = "/tmp/bloom.bin"; + int capacity = 1; static char *kwlist[] = {"capacity", "error_rate", "filepath", NULL}; if (! PyArg_ParseTupleAndKeywords(args, kwds, "|ids", kwlist, &capacity, &error_rate, &filepath)) { return -1; } + + if (capacity < 1){ + PyErr_SetString(DabloomsError, "Bloom creation failed: capacity must be greater than zero"); + return -1; + } + if (error_rate > 1){ + PyErr_SetString(DabloomsError, "Bloom creation failed: error_rate must be between 0 and 1"); + return -1; + } + if(!(filepath && strlen(filepath))){ + PyErr_SetString(DabloomsError, "Bloom creation failed: filepath required"); + return -1; + } self->filter = new_scaling_bloom(capacity, error_rate, filepath); @@ -171,6 +194,7 @@ static PyTypeObject DabloomsType = { 0, /*tp_descr_get*/ 0, /*tp_descr_set*/ 0, /*tp_dictoffset*/ +// 0, /*tp_init*/ (initproc)Dablooms_init, /*tp_init*/ 0, /*tp_alloc*/ Dablooms_new, /*tp_new*/ @@ -220,4 +244,8 @@ PyMODINIT_FUNC initpydablooms(void) Py_INCREF(&DabloomsType); PyModule_AddObject(m, "Dablooms", (PyObject *)&DabloomsType); + + DabloomsError = PyErr_NewException("Dablooms.Error", NULL, NULL); + Py_INCREF(DabloomsError); + PyModule_AddObject(m, "error", DabloomsError); } From 408768f4b751d2315d7a5231cdb416393ac81666 Mon Sep 17 00:00:00 2001 From: Martin Thomas Date: Thu, 9 Jan 2014 11:03:03 -0600 Subject: [PATCH 2/4] error_rate should be positive and less than 1 --- pydablooms/pydablooms.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pydablooms/pydablooms.c b/pydablooms/pydablooms.c index 167112a..b543cbf 100644 --- a/pydablooms/pydablooms.c +++ b/pydablooms/pydablooms.c @@ -54,7 +54,7 @@ static int Dablooms_init(Dablooms *self, PyObject *args, PyObject *kwds) PyErr_SetString(DabloomsError, "Bloom creation failed: capacity must be greater than zero"); return -1; } - if (error_rate > 1){ + if (error_rate > 1 || error_rate < 0){ PyErr_SetString(DabloomsError, "Bloom creation failed: error_rate must be between 0 and 1"); return -1; } From b44a8b8905d5381c1dc6865759c3b52d592d0965 Mon Sep 17 00:00:00 2001 From: Martin Thomas Date: Thu, 9 Jan 2014 11:08:21 -0600 Subject: [PATCH 3/4] Cleanup prior to pull request --- pydablooms/pydablooms.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/pydablooms/pydablooms.c b/pydablooms/pydablooms.c index b543cbf..59b78a8 100644 --- a/pydablooms/pydablooms.c +++ b/pydablooms/pydablooms.c @@ -14,7 +14,7 @@ typedef struct { static void Dablooms_dealloc(Dablooms *self) { if(self->filter) - free_scaling_bloom(self->filter); + free_scaling_bloom(self->filter); self->ob_type->tp_free((PyObject *)self); } @@ -30,12 +30,6 @@ static PyObject *Dablooms_new(PyTypeObject *type, PyObject *args, PyObject *kwds self->filter = NULL; return (PyObject *) self; -// if(! Dablooms_init(self, args, kwds)) -// return (PyObject *) self; -// else { -// PyErr_SetString(PyExc_ZeroDivisionError, "Bloom creation failed"); -// return NULL; -// } } static int Dablooms_init(Dablooms *self, PyObject *args, PyObject *kwds) @@ -194,7 +188,6 @@ static PyTypeObject DabloomsType = { 0, /*tp_descr_get*/ 0, /*tp_descr_set*/ 0, /*tp_dictoffset*/ -// 0, /*tp_init*/ (initproc)Dablooms_init, /*tp_init*/ 0, /*tp_alloc*/ Dablooms_new, /*tp_new*/ From fbb513606464d0144d945cb2339dadd57f74fa8f Mon Sep 17 00:00:00 2001 From: Martin Thomas Date: Thu, 9 Jan 2014 16:44:28 -0600 Subject: [PATCH 4/4] Added parameter defauls and checking to load_dabloom Removed extraneous forward declaration. Updated load_dabloom to have default values for named parameters. Check values for sanity (no negative capacity etc). --- pydablooms/pydablooms.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/pydablooms/pydablooms.c b/pydablooms/pydablooms.c index 59b78a8..8b69187 100644 --- a/pydablooms/pydablooms.c +++ b/pydablooms/pydablooms.c @@ -18,8 +18,6 @@ static void Dablooms_dealloc(Dablooms *self) self->ob_type->tp_free((PyObject *)self); } -static int Dablooms_init(Dablooms *self, PyObject *args, PyObject *kwds); - static PyObject *Dablooms_new(PyTypeObject *type, PyObject *args, PyObject *kwds) { Dablooms *self; @@ -36,7 +34,7 @@ static int Dablooms_init(Dablooms *self, PyObject *args, PyObject *kwds) { double error_rate = 0.1; const char *filepath = "/tmp/bloom.bin"; - int capacity = 1; + int capacity = 1; // dropped the unsigned modifier to avoid implicit conversion from negative static char *kwlist[] = {"capacity", "error_rate", "filepath", NULL}; if (! PyArg_ParseTupleAndKeywords(args, kwds, "|ids", kwlist, @@ -196,18 +194,36 @@ static PyTypeObject DabloomsType = { static PyObject *load_dabloom(PyTypeObject *type, PyObject *args, PyObject *kwds) { Dablooms *self = (Dablooms *)PyObject_New(Dablooms, &DabloomsType); - double error_rate; - const char *filepath; - unsigned int capacity; + double error_rate = 0.1; + const char *filepath = "/tmp/bloom.bin"; + int capacity = 1; // dropped the unsigned modifier to avoid implicit conversion from negative + int result = 0; static char *kwlist[] = {"capacity", "error_rate", "filepath", NULL}; - + if (! PyArg_ParseTupleAndKeywords(args, kwds, "|ids", kwlist, &capacity, &error_rate, &filepath)) { return NULL; } - - self->filter = new_scaling_bloom_from_file(capacity, error_rate, filepath); - return (PyObject *) self; + + if (capacity < 1){ + PyErr_SetString(DabloomsError, "Bloom creation failed: capacity must be greater than zero"); + result = -1; + } + else if (error_rate > 1 || error_rate < 0){ + PyErr_SetString(DabloomsError, "Bloom creation failed: error_rate must be between 0 and 1"); + result = -1; + } + else if(!(filepath && strlen(filepath))){ + PyErr_SetString(DabloomsError, "Bloom creation failed: filepath required"); + result = -1; + } + + if (!result){ + self->filter = new_scaling_bloom_from_file(capacity, error_rate, filepath); + return (PyObject *) self; + } + Dablooms_dealloc(self); + return NULL; } static PyMethodDef pydablooms_methods[] = {