Nir Soffer
2022-Feb-03 22:49 UTC
[Libguestfs] [libnbd PATCH v2 1/5] python: tests: Fix error handling
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 errorThis 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 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.1Nir
Eric Blake
2022-Feb-03 23:12 UTC
[Libguestfs] [libnbd PATCH v2 1/5] python: tests: Fix error handling
On Fri, Feb 04, 2022 at 12:49:27AM +0200, 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. > >> > @@ -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 would use: > > if error != 0: > raise RuntimeError(f"read: {os.strerror(error)}") > > Which also teaches the reader that error is an errno value.Will do (I trust your python expertise more than my own ;) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Laszlo Ersek
2022-Feb-04 08:36 UTC
[Libguestfs] [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 >