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

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
         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

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 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