On Sun, May 5, 2019 at 4:44 PM Amit Bawer abawer.com@gmail.com wrote:
As stated in sanlock module help: The disks must be in the format: [(path, offset), ... ]
Signed-off-by: Amit Bawer abawer@redhat.com
python/sanlock.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/python/sanlock.c b/python/sanlock.c index a1215bd..aefcc3b 100644 --- a/python/sanlock.c +++ b/python/sanlock.c @@ -109,8 +109,6 @@ __parse_resource(PyObject *obj, struct sanlk_resource **res_ret) __set_exception(EINVAL, "Invalid resource offset"); goto exit_fail; }
} else if (PyString_Check(tuple)) {
p = PyString_AsString(tuple); }
This works, and we will fail later in:
114 if (p == NULL) {
115 __set_exception(EINVAL, "Invalid resource path"); 116 goto exit_fail; 117 } But the flow could be more clear, and we can simplify the code while removing the undocumented value.
Also, we should really fail with ValueError instead of SanlockException, since the user passed invalid value, and the invalid value should apear in the exception string, like we do when you pass invalid align value.
So I think what we should have is something like this (untested):
disk = PyList_GetItem(obj, i);
if (!PyTuple_Check(disk)) { const char *s = ""; PyObject *r = PyObject_Repr(disk); if (r) s = PyString_AsString(r); PyErr_Format(PyExc_ValueError, "Invalid disk value: %s", s); Py_XDECREF(r); goto exit_fail; }
And then we can have the rest of the checks outside of the if block, simplifying the flow.
We should probably have a helper for raising a value error with invalid object, including the object repr() in the error message, so we can do:
if (!PyTuple_Check(disk)) { set_value_error("invalid disk value: %s", disk); goto exit_fail; }
We can use this later for handling bad path or offset values.
Finally, we need a simple test verifying that we actually fail with the expected exception type and error message when providing invalid value, similar to the new tests for invalid align/sector.
Nir