On Wednesday, 19 May 2021 11:32:15 CEST Nir Soffer wrote:
On Wed, May 19, 2021 at 11:32 AM Vojtech Juranek
<vjuranek(a)redhat.com>
wrote:
> NB: if somewon wants to review in web UI, see
>
https://github.com/nirs/sanlock/commit/683b9d6d46649da9f5650c8e467d07e350d
> 1d6ed
Let's keep the discussion in one place. The mailing list is the best
way to review
and discuss stuff.
> On Tuesday, 18 May 2021 15:29:02 CEST Nir Soffer wrote:
> > Use sanlock_inquire() to query the resource held by the current process
> > (using the slkfd= argument) or held by another program (using the pid=
> > argument).
> >
> > When using the slkfd= argument, we communicate with sanlock daemon using
> > slkfd, ensuring that the current process is connected to sanlock. If the
> > current process is not connected, sanlock assumes that the process is
> > dead, and release all the leases acquired by the process.
> >
> > When using the pid= argument, the function opens a new socket to sanlock
> > daemon and query the status of resources owned by specified pid.
> >
> > In both cases the information comes from sanlock daemon, without
> > accessing storage. To verify storage content, the caller should use
> > read_resource() and read_resource_owners().
> >
> > The call returns list of resources dicts that can be used for verifying
> > that sanlock state matches the program state.
> >
> > sanlock_inquire() reports the SANLOCK_RES_LVER or sanlock.RES_SHARED
> > flags in the resource flags field. Add the field to the returned dict
> > and add sanlock constants for the flag.
> >
> > The resource flags are needed if you want to restore a lease after it
> > was released, ensuring that nobody else acquired the lease after it was
> > released. This flow is used by libvirt using libsanlock. With this
> > change we can implement the same flow using the python binding.
> >
> > Signed-off-by: Nir Soffer <nsoffer(a)redhat.com>
> > ---
> >
> > python/sanlock.c | 173 +++++++++++++++++++++++++++++++++++++++++++
> > tests/python_test.py | 84 +++++++++++++++++++++
> > 2 files changed, 257 insertions(+)
> >
> > diff --git a/python/sanlock.c b/python/sanlock.c
> > index 67d34fc..238d172 100644
> > --- a/python/sanlock.c
> > +++ b/python/sanlock.c
> >
> > @@ -323,6 +323,88 @@ exit_fail:
> > return NULL;
> >
> > }
> >
> > +/* Convert disks array to list of tuples. */
> > +static PyObject *
> > +disks_to_list(struct sanlk_disk *disks, uint32_t disks_count)
> > +{
> > + PyObject *result = NULL;
> > + PyObject *disk = NULL;
> > +
> > + result = PyList_New(disks_count);
> > + if (result == NULL)
> > + return NULL;
A memory error was set in this case...
Thanks, I didn't realize this. This was the case which was unclear to me (or
better to say which I misunderstood).
LGTM, +1 to the patch
> > +
> > + for (uint32_t i = 0; i < disks_count; i++) {
> > + disk = Py_BuildValue(
> > + "(s,K)",
> > + disks[i].path,
> > + disks[i].offset);
> > + if (disk == NULL)
> > + goto exit_fail;
This can fail with memory error, value error (converting offset to integer)
or unicode error (decoding path).
> > +
> > + /* Steals reference to disk. */
> > + if (PyList_SetItem(result, i, disk) != 0)
> > + goto exit_fail;
This can fail (not sure how)
> > +
> > + disk = NULL;
> > + }
> > +
> > + return result;
> > +
> > +exit_fail:
> > + Py_XDECREF(result);
> > + Py_XDECREF(disk);
> > +
> > + return NULL;
> > +}
> > +
> > +/* Convert resources array returned from sanlock_inquire() to list of
> > resource + * dicts. */
> > +static PyObject *
> > +resources_to_list(struct sanlk_resource **res, int res_count)
> > +{
> > + PyObject *result = NULL;
> > + PyObject *info = NULL;
> > + PyObject *disks = NULL;
> > +
> > + if ((result = PyList_New(res_count)) == NULL)
> > + return NULL;
> > +
> > + for (int i = 0; i < res_count; i++) {
> > + disks = disks_to_list(res[i]->disks, res[i]->num_disks);
> > + if (disks == NULL)
> > + goto exit_fail;
>
> why do we fail here? NULL is possible return value from disks_to_list(),
> so we should handle it and not fail (possibly return NULL?)
Getting NULL from disks_to_list() means disk_to_list() has failed and
set an error. In this case we need to return NULL from py_inquire(), and
on the python side the error set in disks_to_list() will be raised.
We cannot return NULL since we already created the result list, and
may have already added some items to the list. We must decrease the
reference count of temporary variables before we return.
This is a common pattern in python c extension code, used everywhere
in this module.
> > + /* Steals reference to disks. */
> > + info = Py_BuildValue(
> > + "{s:y,s:y,s:k,s:K,s:N}",
> > + "lockspace", res[i]->lockspace_name,
> > + "resource", res[i]->name,
> > + "flags", res[i]->flags,
> > + "version", res[i]->lver,
> > + "disks", disks);
> > + if (info == NULL)
> > + goto exit_fail;
> > +
> > + disks = NULL;
> > +
> > + /* Steals reference to info. */
> > + if (PyList_SetItem(result, i, info) != 0)
> > + goto exit_fail;
> > +
> > + info = NULL;
> > + }
> > +
> > + return result;
> > +
> > +exit_fail:
> > + Py_XDECREF(result);
> > + Py_XDECREF(info);
> > + Py_XDECREF(disks);
> > +
> > + return NULL;
> > +}
> > +
> >
> > /* register */
> > PyDoc_STRVAR(pydoc_register, "\
> > register() -> int\n\
> >
> > @@ -1062,6 +1144,89 @@ finally:
> > Py_RETURN_NONE;
> >
> > }
> >
> > +/* inquire */
> > +PyDoc_STRVAR(pydoc_inquire, "\
> > +inquire(slkfd=fd, pid=owner)\n\
> > +Return list of resources held by current process (using the slkfd \n\
> > +argument to specify the sanlock file descriptor) or for another \n\
> > +process (using the pid argument).\n\
> > +\n\
> > +Does not access storage. To learn about resource state on storage,\n\
> > +use sanlock.read_resource() and sanlock.read_resource_owners().\n\
> > +\n\
> > +Arguments\n\
> > + slkfd (int): The file descriptor returned from
> > sanlock.register().\n\
> > + pid (int): The program pid to query.\n\
> > +\n\
> > +Returns\n\
> > + List of resource dicts with the following keys:\n\
> > + lockspace (bytes): lockspace name\n\
> > + resource (bytes): resource name\n\
> > + flags (int): resource flags (sanlock.RES_*)\n\
> > + version (int): resource version\n\
> > + disks (list): list of disk tuples (path, offset)\n\
> > +");
> > +
> > +static PyObject *
> > +py_inquire(PyObject *self __unused, PyObject *args, PyObject *keywds)
> > +{
> > + int sanlockfd = -1;
> > + int pid = -1;
> > + char *kwlist[] = {"slkfd", "pid", NULL};
> > + int rv = -1;
> > +
> > + /* sanlock_inquire() return values. */
> > + int res_count = 0;
> > + char *res_state = NULL;
> > +
> > + /* Array of resoruces parsed from res_state. */
> > + struct sanlk_resource **res_arr = NULL;
> > +
> > + /* List of resource dicts. */
> > + PyObject *result = NULL;
> > +
> > + if (!PyArg_ParseTupleAndKeywords(
> > + args, keywds, "|ii", kwlist, &sanlockfd, &pid))
{
> > + return NULL;
> > + }
> > +
> > + /* Check if any of the slkfd or pid parameters was given. */
> > + if (sanlockfd == -1 && pid == -1) {
> > + set_sanlock_error(-EINVAL, "Invalid slkfd and pid values");
> > + return NULL;
> > + }
> > +
> > + /* Inquire sanlock (gil disabled) */
> > + Py_BEGIN_ALLOW_THREADS
> > + rv = sanlock_inquire(sanlockfd, pid, 0, &res_count, &res_state);
> > + Py_END_ALLOW_THREADS
> > +
> > + if (rv != 0) {
> > + set_sanlock_error(rv, "Inquire error");
> > + return NULL;
> > + }
> > +
> > + if (res_count > 0) {
> > + rv = sanlock_state_to_args(res_state, &res_count, &res_arr);
> > + if (rv != 0) {
> > + /* TODO: Include res_state in the error. */
> > + set_sanlock_error(rv, "Error parsing inquire state
> > string");
> > + goto finally;
> > + }
> > + }
> > +
> > + result = resources_to_list(res_arr, res_count);
> > +
> > +finally:
> > + free(res_state);
> > +
> > + for (int i = 0; i < res_count; i++)
> > + free(res_arr[i]);
> > + free(res_arr);
> > +
> > + return result;
> > +}
> > +
> >
> > /* release */
> > PyDoc_STRVAR(pydoc_release, "\
> > release(lockspace, resource, disks [, slkfd=fd, pid=owner])\n\
> >
> > @@ -1752,6 +1917,8 @@ sanlock_methods[] = {
> >
> > METH_VARARGS|METH_KEYWORDS,
> > pydoc_read_resource_owners},
> >
> > {"acquire", (PyCFunction) py_acquire,
> >
> > METH_VARARGS|METH_KEYWORDS, pydoc_acquire},
> >
> > + {"inquire", (PyCFunction) py_inquire,
> > + METH_VARARGS|METH_KEYWORDS, pydoc_inquire},
> >
> > {"release", (PyCFunction) py_release,
> >
> > METH_VARARGS|METH_KEYWORDS, pydoc_release},
> >
> > {"request", (PyCFunction) py_request,
> >
> > @@ -1850,6 +2017,12 @@ module_init(PyObject* m)
> >
> > if (PyModule_AddIntConstant(m, "SETEV_ALL_HOSTS",
> >
> > SANLK_SETEV_ALL_HOSTS)) return -1;
> >
> > + /* sanlock_inquire() result resource flags */
> > + if (PyModule_AddIntConstant(m, "RES_LVER", SANLK_RES_LVER))
> > + return -1;
> > + if (PyModule_AddIntConstant(m, "RES_SHARED", SANLK_RES_SHARED))
> > + return -1;
> > +
> >
> > /* Tuples with supported sector size and alignment values */
> >
> > PyObject *sector = Py_BuildValue("ii", SECTOR_SIZE_512,
> >
> > SECTOR_SIZE_4K); diff --git a/tests/python_test.py
> > b/tests/python_test.py
> > index 58a22c7..caf2f3e 100644
> > --- a/tests/python_test.py
> > +++ b/tests/python_test.py
> > @@ -479,6 +479,90 @@ def test_acquire_release_resource(tmpdir,
> > sanlock_daemon, size, offset): assert owners == []
> >
> >
> > +(a)pytest.mark.parametrize("res_name", [
> > + "ascii",
> > + "\u05d0", # Hebrew Alef
> > +])
> > +def test_inquire(tmpdir, sanlock_daemon, res_name):
> > + ls_path = str(tmpdir.join("ls_name"))
> > + util.create_file(ls_path, MiB)
> > +
> > + res_path = str(tmpdir.join(res_name))
> > + util.create_file(res_path, 10 * MiB)
> > +
> > + fd = sanlock.register()
> > +
> > + # No lockspace yet.
> > + assert sanlock.inquire(slkfd=fd) == []
> > +
> > + sanlock.write_lockspace(b"ls_name", ls_path, offset=0,
iotimeout=1)
> > + sanlock.add_lockspace(b"ls_name", 1, ls_path, offset=0,
> > iotimeout=1)
> > +
> > + # No resources created yet.
> > + assert sanlock.inquire(slkfd=fd) == []
> > +
> > + resources = [
> > + # name, offset, acquire
> > + (b"res-0", 0 * MiB, True),
> > + (b"res-1", 1 * MiB, False),
> > + (b"res-2", 2 * MiB, True),
> > + (b"res-8", 8 * MiB, False),
> > + (b"res-9", 9 * MiB, True),
> > + ]
> > +
> > + for res_name, res_offset, acquire in resources:
> > + sanlock.write_resource(b"ls_name", res_name, [(res_path,
> > res_offset)]) +
> > + # No resource acquired yet.
> > + assert sanlock.inquire(slkfd=fd) == []
> > +
> > + # Acquire resources.
> > + for res_name, res_offset, acquire in resources:
> > + if acquire:
> > + sanlock.acquire(
> > + b"ls_name", res_name, [(res_path, res_offset)],
> > slkfd=fd)
> > +
> > + time.sleep(1)
> > +
> > + expected = [
> > + {
> > + "lockspace": b"ls_name",
> > + "resource": b"res-0",
> > + "flags": sanlock.RES_LVER,
> > + "version": 1,
> > + "disks": [(res_path, 0 * MiB)],
> > + },
> > + {
> > + "lockspace": b"ls_name",
> > + "resource": b"res-2",
> > + "flags": sanlock.RES_LVER,
> > + "version": 1,
> > + "disks": [(res_path, 2 * MiB)],
> > + },
> > + {
> > + "lockspace": b"ls_name",
> > + "resource": b"res-9",
> > + "flags": sanlock.RES_LVER,
> > + "version": 1,
> > + "disks": [(res_path, 9 * MiB)],
> > + },
> > + ]
> > +
> > + # Check acquired resources using snlkfd.
> > + assert sanlock.inquire(slkfd=fd) == expected
> > +
> > + # Check acquired resources using pid.
> > + assert sanlock.inquire(pid=os.getpid()) == expected
> > +
> > + for res_name, res_offset, acquire in resources:
> > + if acquire:
> > + sanlock.release(
> > + b"ls_name", res_name, [(res_path, res_offset)],
> > slkfd=fd)
> > +
> > + # All resource released.
> > + assert sanlock.inquire(slkfd=fd) == []
> > +
> > +
> >
> > @pytest.mark.parametrize("align, sector", [
> >
> > # Invalid alignment
> > (KiB, sanlock.SECTOR_SIZE[0]),