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

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.

Laszlo
> 
> I would use:
> 
>     if error != 0:
>         raise RuntimeError(f"read: {os.strerror(error)}")
> 
> Which also teaches the reader that error is an errno value.
> 
>>          global bytes_read
>>          bytes_read += buf.size()
>>          wr = (buf, offset)
>> @@ -46,6 +47,7 @@ def asynch_copy(src, dst):
>>      # This callback is called when any pwrite to the destination
>>      # has completed.
>>      def write_completed(buf, error):
>> +        assert not error
>>          global bytes_written
>>          bytes_written += buf.size()
>>          # By returning 1 here we auto-retire the pwrite command.
>> --
>> 2.34.1
> 
> Nir
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
>

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.

Nir