Add sector and align parameters to read and write lockspace and resource calls to test_write_lockspace and test_write_resource tests to test that these parameters are accepted. Other tests still use calls without these parameters to test the defaults. --- tests/python_test.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/tests/python_test.py b/tests/python_test.py index 5029924..6393222 100644 --- a/tests/python_test.py +++ b/tests/python_test.py @@ -34,9 +34,12 @@ def test_write_lockspace(tmpdir, sanlock_daemon, size, offset): path = str(tmpdir.join("lockspace")) util.create_file(path, size)
- sanlock.write_lockspace("name", path, offset=offset, iotimeout=1) + sanlock.write_lockspace( + "name", path, offset=offset, iotimeout=1, align=sanlock.ALIGN1M, + sector=sanlock.SECTOR512)
- ls = sanlock.read_lockspace(path, offset=offset) + ls = sanlock.read_lockspace( + path, offset=offset, align=sanlock.ALIGN1M, sector=sanlock.SECTOR512) assert ls == {"iotimeout": 1, "lockspace": "name"}
acquired = sanlock.inq_lockspace( @@ -64,9 +67,12 @@ def test_write_resource(tmpdir, sanlock_daemon, size, offset): util.create_file(path, size) disks = [(path, offset)]
- sanlock.write_resource("ls_name", "res_name", disks) + sanlock.write_resource( + "ls_name", "res_name", disks, align=sanlock.ALIGN1M, + sector=sanlock.SECTOR512)
- res = sanlock.read_resource(path, offset=offset) + res = sanlock.read_resource( + path, offset=offset, align=sanlock.ALIGN1M, sector=sanlock.SECTOR512) assert res == { "lockspace": "ls_name", "resource": "res_name",
Currently appropriate sanlock flags are accepted for sector size and alignment parameters. This can be confusing for users, which may try to pass values intead of flags and this approach also expose sanlock internal implentation details.
In order to have consisten and more intuitive API, values should be accepted for sector size and alignment parameters. This patch replaces flags by values in calls which accept sector size and alignment.
This patch also adds validation of provided values. If the value is not the supported one, error with error code EINVAL is thrown.
The supported values are: sector: 512, 4096 align: 1048576, 2097152, 4194304, 8388608 --- python/example.py | 4 +- python/sanlock.c | 137 +++++++++++++++++++++++++++++++++++-------- tests/python_test.py | 12 ++-- 3 files changed, 117 insertions(+), 36 deletions(-)
diff --git a/python/example.py b/python/example.py index 4fe845d..cb8f3e9 100644 --- a/python/example.py +++ b/python/example.py @@ -29,10 +29,10 @@ def main(): fd = sanlock.register()
print "Initializing '%s'" % (LOCKSPACE_NAME,) - sanlock.write_lockspace(LOCKSPACE_NAME, disk, max_hosts=0, iotimeout=0, align=sanlock.ALIGN1M, sector=sanlock.SECTOR512) + sanlock.write_lockspace(LOCKSPACE_NAME, disk, max_hosts=0, iotimeout=0, align=1048576, sector=512)
print "Initializing '%s' on '%s'" % (RESOURCE_NAME, LOCKSPACE_NAME) - sanlock.write_resource(LOCKSPACE_NAME, RESOURCE_NAME, SNLK_DISKS, align=sanlock.ALIGN1M, sector=sanlock.SECTOR512) + sanlock.write_resource(LOCKSPACE_NAME, RESOURCE_NAME, SNLK_DISKS, align=1048576, sector=512)
print "Acquiring the id '%i' on '%s'" % (HOST_ID, LOCKSPACE_NAME) sanlock.add_lockspace(LOCKSPACE_NAME, HOST_ID, disk) diff --git a/python/sanlock.c b/python/sanlock.c index 2ad4497..323f1bd 100644 --- a/python/sanlock.c +++ b/python/sanlock.c @@ -53,6 +53,7 @@ __set_exception(int en, char *msg) if (en < 0 && en > -200) { en = -en; err_name = strerror(en); + } else { /* Safe to call without releasing the GIL. */ err_name = sanlock_strerror(en); @@ -135,6 +136,54 @@ exit_fail: return -1; }
+enum sector_size {SECTOR512 = 512, SECTOR4K = 4096}; + +static uint32_t +__sector_to_flag(int sector) +{ + uint32_t flag = 0; + + switch (sector) { + case SECTOR512: + flag = SANLK_LSF_SECTOR512; + break; + case SECTOR4K: + flag = SANLK_LSF_SECTOR4K; + break; + default: + __set_exception(EINVAL, "Invalid alignment value");; + } + + return flag; +} + +enum alignment {ALIGN1M = 1048576, ALIGN2M = 2097152, ALIGN4M = 4194304, ALIGN8M = 8388608}; + +static uint32_t +__align_to_flag(long align) +{ + uint32_t flag = 0; + + switch (align) { + case ALIGN1M: + flag = SANLK_RES_ALIGN1M; + break; + case ALIGN2M: + flag = SANLK_RES_ALIGN2M; + break; + case ALIGN4M: + flag = SANLK_RES_ALIGN4M; + break; + case ALIGN8M: + flag = SANLK_RES_ALIGN8M; + break; + default: + __set_exception(EINVAL, "Invalid alignment value");; + } + + return flag; +} + static PyObject * __hosts_to_list(struct sanlk_host *hss, int hss_count) { @@ -361,15 +410,17 @@ exit_fail: /* write_lockspace */ PyDoc_STRVAR(pydoc_write_lockspace, "\ write_lockspace(lockspace, path, offset=0, max_hosts=0, iotimeout=0, \ -align=ALIGN1M, sector=SECTOR512)\n\ -Initialize or update a device to be used as sanlock lockspace."); +align=1048576, sector=512)\n\ +Initialize or update a device to be used as sanlock lockspace.\n\ +Align can be one of (1048576, 2097152, 4194304, 8388608).\n\ +Sector can be on of (512, 4096).");
static PyObject * py_write_lockspace(PyObject *self __unused, PyObject *args, PyObject *keywds) { - int rv, max_hosts = 0; + int rv, max_hosts = 0, sector=SECTOR512; + long align=ALIGN1M; uint32_t io_timeout = 0; - uint32_t align=SANLK_LSF_ALIGN1M, sector=SANLK_LSF_SECTOR512; const char *lockspace, *path; struct sanlk_lockspace ls;
@@ -380,7 +431,7 @@ py_write_lockspace(PyObject *self __unused, PyObject *args, PyObject *keywds) memset(&ls, 0, sizeof(struct sanlk_lockspace));
/* parse python tuple */ - if (!PyArg_ParseTupleAndKeywords(args, keywds, "ss|kiIII", kwlist, + if (!PyArg_ParseTupleAndKeywords(args, keywds, "ss|kiIli", kwlist, &lockspace, &path, &ls.host_id_disk.offset, &max_hosts, &io_timeout, &align, §or)) { return NULL; @@ -391,9 +442,17 @@ py_write_lockspace(PyObject *self __unused, PyObject *args, PyObject *keywds) strncpy(ls.host_id_disk.path, path, SANLK_PATH_LEN - 1);
/* set alignment/sector flags */ - ls.flags |= align; - ls.flags |= sector; + uint32_t flag ; + if ((flag = __align_to_flag(align)) != 0) + ls.flags |= flag; + else + return NULL;
+ if ((flag = __sector_to_flag(sector)) != 0) + ls.flags |= flag; + else + return NULL; + /* write sanlock lockspace (gil disabled) */ Py_BEGIN_ALLOW_THREADS rv = sanlock_write_lockspace(&ls, max_hosts, 0, io_timeout); @@ -409,15 +468,15 @@ py_write_lockspace(PyObject *self __unused, PyObject *args, PyObject *keywds)
/* read_lockspace */ PyDoc_STRVAR(pydoc_read_lockspace, "\ -read_lockspace(path, offset=0, align=ALIGN1M, sector=SECTOR512)\n -> dict\n\ +read_lockspace(path, offset=0, align=1048576, sector=512)\n -> dict\n\ Read the lockspace information from a device at a specific offset.");
static PyObject * py_read_lockspace(PyObject *self __unused, PyObject *args, PyObject *keywds) { - int rv; + int rv, sector=SECTOR512; + long align=ALIGN1M; uint32_t io_timeout = 0; - uint32_t align=SANLK_LSF_ALIGN1M, sector=SANLK_LSF_SECTOR512; const char *path; struct sanlk_lockspace ls; PyObject *ls_info = NULL, *ls_entry = NULL; @@ -428,7 +487,7 @@ py_read_lockspace(PyObject *self __unused, PyObject *args, PyObject *keywds) memset(&ls, 0, sizeof(struct sanlk_lockspace));
/* parse python tuple */ - if (!PyArg_ParseTupleAndKeywords(args, keywds, "s|kII", kwlist, + if (!PyArg_ParseTupleAndKeywords(args, keywds, "s|kli", kwlist, &path, &ls.host_id_disk.offset, &align, §or)) { return NULL; } @@ -437,8 +496,16 @@ py_read_lockspace(PyObject *self __unused, PyObject *args, PyObject *keywds) strncpy(ls.host_id_disk.path, path, SANLK_PATH_LEN - 1);
/* set alignment/sector flags */ - ls.flags |= align; - ls.flags |= sector; + uint32_t flag ; + if ((flag = __align_to_flag(align)) != 0) + ls.flags |= flag; + else + return NULL; + + if ((flag = __sector_to_flag(sector)) != 0) + ls.flags |= flag; + else + return NULL;
/* read sanlock lockspace (gil disabled) */ Py_BEGIN_ALLOW_THREADS @@ -481,14 +548,14 @@ exit_fail:
/* read_resource */ PyDoc_STRVAR(pydoc_read_resource, "\ -read_resource(path, offset=0, align=ALIGN1M, sector=SECTOR512) -> dict\n\ +read_resource(path, offset=0, align=1048576, sector=512) -> dict\n\ Read the resource information from a device at a specific offset.");
static PyObject * py_read_resource(PyObject *self __unused, PyObject *args, PyObject *keywds) { - int rv, rs_len; - uint32_t align=SANLK_RES_ALIGN1M, sector=SANLK_RES_SECTOR512; + int rv, rs_len, sector=SECTOR512; + long align=ALIGN1M; const char *path; struct sanlk_resource *rs; PyObject *rs_info = NULL, *rs_entry = NULL; @@ -509,7 +576,7 @@ py_read_resource(PyObject *self __unused, PyObject *args, PyObject *keywds) rs->num_disks = 1;
/* parse python tuple */ - if (!PyArg_ParseTupleAndKeywords(args, keywds, "s|kII", kwlist, + if (!PyArg_ParseTupleAndKeywords(args, keywds, "s|kli", kwlist, &path, &(rs->disks[0].offset), &align, §or)) { goto exit_fail; } @@ -518,8 +585,16 @@ py_read_resource(PyObject *self __unused, PyObject *args, PyObject *keywds) strncpy(rs->disks[0].path, path, SANLK_PATH_LEN - 1);
/* set alignment/sector flags */ - rs->flags |= align; - rs->flags |= sector; + uint32_t flag ; + if ((flag = __align_to_flag(align)) != 0) + rs->flags |= flag; + else + goto exit_fail; + + if ((flag = __sector_to_flag(sector)) != 0) + rs->flags |= flag; + else + goto exit_fail;
/* read sanlock resource (gil disabled) */ Py_BEGIN_ALLOW_THREADS @@ -573,17 +648,19 @@ exit_fail: /* write_resource */ PyDoc_STRVAR(pydoc_write_resource, "\ write_resource(lockspace, resource, disks, max_hosts=0, num_hosts=0, \ -clear=False, align=ALIGN1M, sector=SECTOR512)\n\ +clear=False, align=1048576, sector=512)\n\ Initialize a device to be used as sanlock resource.\n\ The disks must be in the format: [(path, offset), ... ].\n\ If clear is True, the resource is cleared so subsequent read will\n\ -return an error."); +return an error.\n\ +Align can be one of (1048576, 2097152, 4194304, 8388608).\n\ +Sector can be on of (512, 4096).");
static PyObject * py_write_resource(PyObject *self __unused, PyObject *args, PyObject *keywds) { - int rv, max_hosts = 0, num_hosts = 0, clear = 0; - uint32_t align=SANLK_RES_ALIGN1M, sector=SANLK_RES_SECTOR512; + int rv, max_hosts = 0, num_hosts = 0, clear = 0, sector=SECTOR512; + long align=ALIGN1M; const char *lockspace, *resource; struct sanlk_resource *rs; PyObject *disks; @@ -593,7 +670,7 @@ py_write_resource(PyObject *self __unused, PyObject *args, PyObject *keywds) "num_hosts", "clear", "align", "sector", NULL};
/* parse python tuple */ - if (!PyArg_ParseTupleAndKeywords(args, keywds, "ssO!|iiiII", + if (!PyArg_ParseTupleAndKeywords(args, keywds, "ssO!|iiili", kwlist, &lockspace, &resource, &PyList_Type, &disks, &max_hosts, &num_hosts, &clear, &align, §or)) { return NULL; @@ -609,8 +686,16 @@ py_write_resource(PyObject *self __unused, PyObject *args, PyObject *keywds) strncpy(rs->name, resource, SANLK_NAME_LEN);
/* set alignment/sector flags */ - rs->flags |= align; - rs->flags |= sector; + uint32_t flag ; + if ((flag = __align_to_flag(align)) != 0) + rs->flags |= flag; + else + goto exit_fail; + + if ((flag = __sector_to_flag(sector)) != 0) + rs->flags |= flag; + else + goto exit_fail;
if (clear) { flags |= SANLK_WRITE_CLEAR; diff --git a/tests/python_test.py b/tests/python_test.py index 6393222..2535af1 100644 --- a/tests/python_test.py +++ b/tests/python_test.py @@ -35,11 +35,9 @@ def test_write_lockspace(tmpdir, sanlock_daemon, size, offset): util.create_file(path, size)
sanlock.write_lockspace( - "name", path, offset=offset, iotimeout=1, align=sanlock.ALIGN1M, - sector=sanlock.SECTOR512) + "name", path, offset=offset, iotimeout=1, align=1048576, sector=512)
- ls = sanlock.read_lockspace( - path, offset=offset, align=sanlock.ALIGN1M, sector=sanlock.SECTOR512) + ls = sanlock.read_lockspace(path, offset=offset, align=1048576, sector=512) assert ls == {"iotimeout": 1, "lockspace": "name"}
acquired = sanlock.inq_lockspace( @@ -68,11 +66,9 @@ def test_write_resource(tmpdir, sanlock_daemon, size, offset): disks = [(path, offset)]
sanlock.write_resource( - "ls_name", "res_name", disks, align=sanlock.ALIGN1M, - sector=sanlock.SECTOR512) + "ls_name", "res_name", disks, align=1048576, sector=512)
- res = sanlock.read_resource( - path, offset=offset, align=sanlock.ALIGN1M, sector=sanlock.SECTOR512) + res = sanlock.read_resource(path, offset=offset, align=1048576, sector=512) assert res == { "lockspace": "ls_name", "resource": "res_name",
Previous patch switched from accepting align and sector flags to values. Exposing internal sanlock flags doesn't make sense now. Tuples with supported sector size and alignment values should be exposed instead. --- python/sanlock.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/python/sanlock.c b/python/sanlock.c index 323f1bd..f367fbd 100644 --- a/python/sanlock.c +++ b/python/sanlock.c @@ -1739,13 +1739,21 @@ initsanlock(void) PYSNLK_INIT_ADD_CONSTANT(SANLK_SETEV_REPLACE_EVENT, "SETEV_REPLACE_EVENT"); PYSNLK_INIT_ADD_CONSTANT(SANLK_SETEV_ALL_HOSTS, "SETEV_ALL_HOSTS");
- /* Sector and align size flags */ - PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_SECTOR512, "SECTOR512"); - PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_SECTOR4K, "SECTOR4K"); - PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN1M, "ALIGN1M"); - PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN2M, "ALIGN2M"); - PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN4M, "ALIGN4M"); - PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN8M, "ALIGN8M"); - #undef PYSNLK_INIT_ADD_CONSTANT + + /* Tuples with supported sector size and alignment values */ + PyObject *sector = PyTuple_New(2); + PyTuple_SetItem(sector, 0, PyInt_FromLong(SECTOR512)); + PyTuple_SetItem(sector, 1, PyInt_FromLong(SECTOR4K)); + if (PyModule_AddObject(py_module, "SECTOR", sector)) + Py_DECREF("SECTOR"); + + PyObject *align = PyTuple_New(4); + PyTuple_SetItem(align, 0, PyInt_FromLong(ALIGN1M)); + PyTuple_SetItem(align, 1, PyInt_FromLong(ALIGN2M)); + PyTuple_SetItem(align, 2, PyInt_FromLong(ALIGN4M)); + PyTuple_SetItem(align, 3, PyInt_FromLong(ALIGN8M)); + if (PyModule_AddObject(py_module, "ALIGN", align)) + Py_DECREF("ALIGN"); + }
--- tests/python_test.py | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
diff --git a/tests/python_test.py b/tests/python_test.py index 2535af1..76e8c96 100644 --- a/tests/python_test.py +++ b/tests/python_test.py @@ -227,3 +227,41 @@ def test_acquire_release_resource(tmpdir, sanlock_daemon, size, offset):
owners = sanlock.read_resource_owners("ls_name", "res_name", disks) assert owners == [] + + +@pytest.mark.parametrize("align, sector", [ + # Invalid alignment + (1024, sanlock.SECTOR[0]), + # Invalid sector size + (sanlock.ALIGN[0], 8192), + # Invalid both alignment and sector size + (3145728, 8192), +]) +def test_write_lockspace_invalid_align_sector( + tmpdir, sanlock_daemon, align, sector): + path = str(tmpdir.join("lockspace")) + util.create_file(path, LOCKSPACE_SIZE) + + with pytest.raises(sanlock.SanlockException) as e: + sanlock.write_lockspace("name", path, align=align, sector=sector) + assert e.value.errno == errno.EINVAL + + +@pytest.mark.parametrize("align, sector", [ + # Invalid alignment + (1024, sanlock.SECTOR[0]), + # Invalid sector size + (sanlock.ALIGN[0], 8192), + # Invalid both alignment and sector size + (3145728, 8192), +]) +def test_write_resource_invalid_align_sector( + tmpdir, sanlock_daemon, align, sector): + path = str(tmpdir.join("resources")) + util.create_file(path, MIN_RES_SIZE) + disks = [(path, 0)] + + with pytest.raises(sanlock.SanlockException) as e: + sanlock.write_resource( + "ls_name", "res_name", disks, align=align, sector=sector) + assert e.value.errno == errno.EINVAL
On Thu, Apr 25, 2019 at 2:57 PM Vojtech Juranek vjuranek@redhat.com wrote:
tests/python_test.py | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
diff --git a/tests/python_test.py b/tests/python_test.py index 2535af1..76e8c96 100644 --- a/tests/python_test.py +++ b/tests/python_test.py @@ -227,3 +227,41 @@ def test_acquire_release_resource(tmpdir, sanlock_daemon, size, offset):
owners = sanlock.read_resource_owners("ls_name", "res_name", disks) assert owners == []
+@pytest.mark.parametrize("align, sector", [
- # Invalid alignment
- (1024, sanlock.SECTOR[0]),
- # Invalid sector size
- (sanlock.ALIGN[0], 8192),
- # Invalid both alignment and sector size
- (3145728, 8192),
I don't think the last value adds anything to the test. Same for the other test.
+])
+def test_write_lockspace_invalid_align_sector(
tmpdir, sanlock_daemon, align, sector):
- path = str(tmpdir.join("lockspace"))
- util.create_file(path, LOCKSPACE_SIZE)
- with pytest.raises(sanlock.SanlockException) as e:
sanlock.write_lockspace("name", path, align=align, sector=sector)
- assert e.value.errno == errno.EINVAL
+@pytest.mark.parametrize("align, sector", [
- # Invalid alignment
- (1024, sanlock.SECTOR[0]),
- # Invalid sector size
- (sanlock.ALIGN[0], 8192),
- # Invalid both alignment and sector size
- (3145728, 8192),
+]) +def test_write_resource_invalid_align_sector(
tmpdir, sanlock_daemon, align, sector):
- path = str(tmpdir.join("resources"))
- util.create_file(path, MIN_RES_SIZE)
- disks = [(path, 0)]
- with pytest.raises(sanlock.SanlockException) as e:
sanlock.write_resource(
"ls_name", "res_name", disks, align=align, sector=sector)
- assert e.value.errno == errno.EINVAL
It would be nice squash this test with the patch adding the validation, and update the patch to use sanlock.ALIGN and sanlock.SECTOR tuples in the patch adding the tuples.
This way we will have only 2 patches for 3.7.1-3.
-- 2.20.1 _______________________________________________ sanlock-devel mailing list -- sanlock-devel@lists.fedorahosted.org To unsubscribe send an email to sanlock-devel-leave@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sanlock-devel@lists.fedorahoste...
sanlock-devel@lists.fedorahosted.org