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
>
>