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

On Fri, Feb 4, 2022 at 10:37 AM Laszlo Ersek <lersek(a)redhat.com&gt; wrote: > > On 02/03/22 23:49, Nir Soffer wrote: >> On Thu, Feb 3, 2022 at 10:26 PM Eric Blake <eblake(a)redhat.com&gt; 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.