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