Re: [PATCH v4 2/2] Expose tuples with supported align and sector values

[Nir Soffer]

On Thu, May 2, 2019 at 4:03 PM Vojtech Juranek <vjuranek(a)redhat.com&gt; wrote:

>  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     | 21 ++++++++++++++-------
>   tests/python_test.py |  8 ++++----
>   2 files changed, 18 insertions(+), 11 deletions(-)
>
>  diff --git a/python/sanlock.c b/python/sanlock.c
>  index d5bff8e..7c4502b 100644
>  --- a/python/sanlock.c
>  +++ b/python/sanlock.c
>  @@ -1717,12 +1717,19 @@ 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 = Py_BuildValue("ii", SECTOR_SIZE_512,
>  SECTOR_SIZE_4K);
>  
Py_BuildValue can fail and return NULL. The import will probably fail with
MemoryError. This is unlikely
but if this happen we will probably segfault in the next line instead of
failing with clear exception.


>  +    if (PyModule_AddObject(py_module, "SECTOR_SIZE",
> sector)) {
>  +        Py_DECREF(sector);
>  +        return;
>  +    }
>  +
>  +    PyObject *align = Py_BuildValue("llll", ALIGNMENT_1M, ALIGNMENT_2M,
>  ALIGNMENT_4M, ALIGNMENT_8M);
>  +    if (PyModule_AddObject(py_module, "ALIGN_SIZE", align)) {
>  +        Py_DECREF(align);
>  +       return;
>  +    }
>  +
>   }
>  diff --git a/tests/python_test.py b/tests/python_test.py
>  index 2e5963f..5ca7a96 100644
>  --- a/tests/python_test.py
>  +++ b/tests/python_test.py
>  @@ -256,9 +256,9 @@ def test_acquire_release_resource(tmpdir,
>  sanlock_daemon, size, offset):
>
>   @pytest.mark.parametrize("align, sector", [
>       # Invalid alignment
>  -    (1024, 512),
>  +    (1024, sanlock.SECTOR_SIZE[0]),
>       # Invalid sector size
>  -    (1048576, 8192),
>  +    (sanlock.ALIGN_SIZE[0], 8192),
>   ])
>   def test_write_lockspace_invalid_align_sector(
>           tmpdir, sanlock_daemon, align, sector):
>  @@ -271,9 +271,9 @@ def test_write_lockspace_invalid_align_sector(
>
>   @pytest.mark.parametrize("align, sector", [
>       # Invalid alignment
>  -    (1024, 512),
>  +    (1024, sanlock.SECTOR_SIZE[0]),
>       # Invalid sector size
>  -    (1048576, 8192),
>  +    (sanlock.ALIGN_SIZE[0], 8192),
>   ])
>   def test_write_resource_invalid_align_sector(
>           tmpdir, sanlock_daemon, align, sector):
>  --
>  2.20.1
>
>  
Otherwise looks fine.

Nir