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/62be33870e2f8517314bf9c7275548e799296f7e/Python/modsupport.c#L638

Is this documented?

Even if it is documented, there is only one thing you should do after a failure - retun.

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