[Libguestfs] [libnbd PATCH v2 1/5] python: tests: Fix error handling

[Laszlo Ersek]

On 02/07/22 02:03, Nir Soffer wrote:
> On Fri, Feb 4, 2022 at 10:37 AM Laszlo Ersek <lersek at redhat.com> wrote:
>>
>> On 02/03/22 23:49, Nir Soffer wrote:
>>> On Thu, Feb 3, 2022 at 10:26 PM Eric Blake <eblake at redhat.com> wrote:
>>>>
>>>> Like a lot of the C examples, the aio copy test ignores read and write
>>>> errors in the completion callback, which can cause silent data
>>>> corruption. The failure in the test is not critical, but this is a bad
>>>> example that may be copied by developers to a real application.
>>>>
>>>> The test dies with an assertion failure now if completion callback
>>>> fails.  Tested with the temporary patch of:
>>>>
>>>> | diff --git i/python/t/590-aio-copy.py w/python/t/590-aio-copy.py
>>>> | index 861fa6c8..4cd64d83 100644
>>>> | --- i/python/t/590-aio-copy.py
>>>> | +++ w/python/t/590-aio-copy.py
>>>> | @@ -117,7 +117,8 @@ src.set_handle_name("src")
>>>> |  dst = nbd.NBD()
>>>> |  dst.set_handle_name("dst")
>>>> |  src.connect_command(["nbdkit", "-s", "--exit-with-parent", "-r",
>>>> | -                     "pattern", "size=%d" % disk_size])
>>>> | +                     "--filter=error", "pattern", "error-pread-rate=1",
>>>> | +                     "size=%d" % disk_size])
>>>> |  dst.connect_command(["nbdkit", "-s", "--exit-with-parent",
>>>> |                       "memory", "size=%d" % disk_size])
>>>> |  asynch_copy(src, dst)
>>>> ---
>>>>  python/t/590-aio-copy.py | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/python/t/590-aio-copy.py b/python/t/590-aio-copy.py
>>>> index 6cde5934..861fa6c8 100644
>>>> --- a/python/t/590-aio-copy.py
>>>> +++ b/python/t/590-aio-copy.py
>>>> @@ -1,5 +1,5 @@
>>>>  # libnbd Python bindings
>>>> -# Copyright (C) 2010-2019 Red Hat Inc.
>>>> +# Copyright (C) 2010-2022 Red Hat Inc.
>>>>  #
>>>>  # This program is free software; you can redistribute it and/or modify
>>>>  # it under the terms of the GNU General Public License as published by
>>>> @@ -36,6 +36,7 @@ def asynch_copy(src, dst):
>>>>      # This callback is called when any pread from the source
>>>>      # has completed.
>>>>      def read_completed(buf, offset, error):
>>>> +        assert not error
>>>
>>> This works for the test, since the test is not compiled
>>> to pyc file, which removes the asserts (like C -DNODBUG)
>>> by default when building rpms.
>>>
>>> If someone will copy this to a real application they will have no
>>> error checking.
>>
>> I consider this a catastrophic bug in python, to be honest. Eliminating
>> assertions should never be done without an explicit request from whoever
>> builds the package.
> 
> I checked this and asserts are not removed automatically.
> 
> They are removed only when using the -O or -OO options:
> 
>     $ python -O -c 'assert 0; print("assert was removed")'
>     assert was removed
> 
> Or:
> 
>     $ PYTHONOPTIMIZE=1 python -c 'assert 0; print("assert was removed")'
>     assert was removed
> 
> Or when compiling modules, if you use the -o1 argument:
> 
>     $ cat test.py
>     assert 0
>     print("assert was removed")
> 
>     $ python -m compileall -o1 test.py
>     Compiling 'test.py'...
> 
>     $ python __pycache__/test.cpython-310.opt-1.pyc
>     assert was removed
> 
> So this is similar to -DNODEBUG, but unlike a compiled program, asserts
> can be removed at runtime without your control.

Thank you for explaining!
Laszlo