On Friday, May 3, 2019, Vojtech Juranek <vjuranek(a)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(a)redhat.com> wrote:
> > On Fri, May 3, 2019 at 12:31 AM Nir Soffer <nsoffer(a)redhat.com> wrote:
> >> On Thu, May 2, 2019 at 4:03 PM Vojtech Juranek <vjuranek(a)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