On Friday, May 3, 2019, Vojtech Juranek <vjuranek@redhat.com> wrote:
> On pátek 3. května 2019 14:10:31 CEST Nir Soffer wrote:
>> On Friday, May 3, 2019, Amit Bawer <abawer@redhat.com> wrote:
>> > On Fri, May 3, 2019 at 12:31 AM Nir Soffer <nsoffer@redhat.com> wrote:
>> >> On Thu, May 2, 2019 at 4:03 PM Vojtech Juranek <vjuranek@redhat.com>
>>
>> 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.
>>
>> > Not sure it will segfault since the last arg in PyModule_AddObject is
>>
>> checked for validity and PyError is set:
>> https://github.com/python/cpython/blob/62be33870e2f8517314bf9c7275548e799296
>> f7e/Python/modsupport.c#L638
>>
>> Is this documented?
>>
>> Even if it is documented, there is only one thing you should do after a
>> failure - retun.
>
> make sense for me, calling PyModule_AddObject with NULL will just tell that
> sector is NULL without any other hint why. So can we all agree on this?
>
>     PyObject *sector = Py_BuildValue("ii", SECTOR_SIZE_512, SECTOR_SIZE_4K);
>     if (!sector)
>         return;
>     if (PyModule_AddObject(py_module, "SECTOR_SIZE", sector)) {
>         Py_DECREF(sector);
>         return;
>     }

+1

>
>
>> Calling another function based on current implementation detail and
>> handling the next failure is sloppy and will make it harder to debug
>> issues, as the original MemoryError will be hidden by a later error.
>>
>> > However IMHO i would use  Py_XDECREF because i like it better and it
>>
>> checks for a NULL pointer first.
>>
>> This is another sloppy usage. This macro is good for the case when object
>> is expected to be NULL during cleanup. Here the object is not expected to
>> be NULL if we check the return value properly.
>>
>> >>> +    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
>
>