[PATCH 0/3] Improve disk validation

[Nir Soffer]

Fix failing disk validation test since python 3.9, and improve error messages
when disk validation fails.

The tests succeeds now on python3.6-python3.11:
https://github.com/nirs/sanlock/actions/runs/3559586867

Nir Soffer (3):
  Improve error message if PyObject_Repr failed
  Improve error messages when validating disks
  Fix disk validation on python >= 3.9

python/sanlock.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

[PATCH 1/3] Improve error message if PyObject_Repr failed

[Nir Soffer]

We reported an empty string:

"Invalid disk: "

Now the error is:

"Invalid disk: (PyObject_Repr failed)"

I think PyObject_Repr() fails because we abuse PyArg_ParseTuple() for
parsing disk tuple.

Signed-off-by: Nir Soffer 
---
 python/sanlock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/python/sanlock.c b/python/sanlock.c
index c481464..db8fb27 100644
--- a/python/sanlock.c
+++ b/python/sanlock.c
@@ -276,21 +276,21 @@ add_align_flag(long align, uint32_t *flags)
     default:
         PyErr_Format(PyExc_ValueError, "Invalid align value: %ld", align);
         return -1;
     }
     return 0;
 }

static void
 set_error(PyObject* exception, const char* format, PyObject* obj)
 {
-    const char* str_rep = "";
+    const char* str_rep = "(PyObject_Repr failed)";
     PyObject* rep = PyObject_Repr(obj);
     if (rep)
         str_rep = pystring_as_cstring(rep);
     PyErr_Format(exception, format, str_rep);
     Py_XDECREF(rep);
 }

static PyObject *
 hosts_to_list(struct sanlk_host *hss, int hss_count)
 {

[PATCH 2/3] Improve error messages when validating disks

[Nir Soffer]

Previously we reported:

Invalid disk: 42

Both when checking that a disk is a tuple, and when failing to parse the
tuple. This makes it hard to debug the failure.

Use unique error message for each case:

Disk is not a tuple: 42
   Cannot parse disk: ('path', 0, 'extra')

Signed-off-by: Nir Soffer 
---
 python/sanlock.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/python/sanlock.c b/python/sanlock.c
index db8fb27..cd6d4b0 100644
--- a/python/sanlock.c
+++ b/python/sanlock.c
@@ -157,27 +157,27 @@ validate_path(PyObject *path)
 }

static int
 parse_single_disk(PyObject* disk, struct sanlk_disk* res_disk)
 {
     int rv = 0;
     PyObject *path = NULL;
     uint64_t offset;

if (!PyTuple_Check(disk)) {
-         set_error(PyExc_ValueError, "Invalid disk %s", disk);
+         set_error(PyExc_ValueError, "Disk is not a tuple: %s", disk);
          goto finally;
     }

if (!PyArg_ParseTuple(disk, "O&K", pypath_converter, &path, &offset)) {
         /* Override the error since it confusing in this context. */
-        set_error(PyExc_ValueError, "Invalid disk %s", disk);
+        set_error(PyExc_ValueError, "Cannot parse disk: %s", disk);
         goto finally;
     }

if (!validate_path(path))
         goto finally;

strncpy(res_disk->path, PyBytes_AsString(path), SANLK_PATH_LEN - 1);
     res_disk->offset = offset;
     rv = 1;

[PATCH 3/3] Fix disk validation on python >= 3.9

[Nir Soffer]

Since python 3.9, when paring tuple with wrong length fails,
PyObject_Repr() fails when we report the error.

I suspect that the reason is abusing PyArg_ParseTyuple() for parsing a
tuple instead of function argument, or maybe this is a bug in python
3.9.

Checking the tuple size before parsing avoids this issue and helps to
show more helpful error message:

Disk length ~= 2: ('path',)

Signed-off-by: Nir Soffer 
---
 python/sanlock.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/python/sanlock.c b/python/sanlock.c
index cd6d4b0..0d05ad6 100644
--- a/python/sanlock.c
+++ b/python/sanlock.c
@@ -161,20 +161,25 @@ parse_single_disk(PyObject* disk, struct sanlk_disk* res_disk)
 {
     int rv = 0;
     PyObject *path = NULL;
     uint64_t offset;

if (!PyTuple_Check(disk)) {
          set_error(PyExc_ValueError, "Disk is not a tuple: %s", disk);
          goto finally;
     }

+    if (PyTuple_Size(disk) != 2) {
+        set_error(PyExc_ValueError, "Disk length != 2: %s", disk);
+        goto finally;
+    }
+
     if (!PyArg_ParseTuple(disk, "O&K", pypath_converter, &path, &offset)) {
         /* Override the error since it confusing in this context. */
         set_error(PyExc_ValueError, "Cannot parse disk: %s", disk);
         goto finally;
     }

if (!validate_path(path))
         goto finally;

strncpy(res_disk->path, PyBytes_AsString(path), SANLK_PATH_LEN - 1);