[Libguestfs] [libnbd PATCH 0/4] Work towards fixing nbdcopy silent corruption

[Laszlo Ersek]

On 02/01/22 20:44, Eric Blake wrote:
> These are the trivial patches modeled after Nir's commit 7ba6ef67,
> fixing further places that fail to check *error during callbacks.

I find these interfaces very difficult to use.

https://libguestfs.org/nbd_aio_block_status.3.html

We have two callbacks here, each takes a *error, and I have zero idea of
the order the callbacks are supposed to be called in.

The last paragraph of the commit message speaks about this, and the code
updates seem to suggest some ordering, but I'm still unsure.

... In fact, I'm now uncertain whether ignoring "err" in virt-v2v's
"lib/utils.ml", function "get_disk_allocated", is safe. It's a "sync"
(not aio) nbd_block_status() call, but the documentation
<https://libguestfs.org/nbd_block_status.3.html> says:

"On entry to the callback, the error parameter contains the errno value
of any previously detected error."

So what should we do then, in "get_disk_allocated"? Return -1
immediately? And if we do so, what is that going to mean for
"NBD.block_status"? Will it raise an exception?

Thanks,
Laszlo

> nbdcopy still has a bigger bug in that it is NOT properly checking for
> *error in the pread/pwrite completion handlers, but fixing that
> gracefully (rather than just quitting the process immediately, as we
> can do in the examples), requires more invasive patches which I will
> be posting separately for review, while the ones posted here are
> trivial enough that I'm pushing them now (commits 23bcea4a..1373d423).
> 
> I also audited other uses of completion callbacks:
> - in tests, closure-lifetimes, errors, server-death, and
>   shutdown-flags are all safe (either the callback returns 0 and the
>   code later calls nbd_aio_command_completed, or we are explicitly
>   testing aspects of completion callbacks)
> - in fuse/operations.c, the completion callback returns 0 and calls
>   nbd_aio_command_completed later
> - in examples/strict-structured-reads.c, the completion callback
>   properly checks *error
> 
> I also checked that nbdkit's nbd plugin properly checks *error.
> 
> Eric Blake (4):
>   examples: aio-connect-read.c: Fix error handling
>   examples: glib-main-loop: don't strand commands when gsource
>     disappears
>   examples: glib-main-loop: Fix error handling
>   copy: Ignore extents if we encountered earlier error
> 
>  examples/aio-connect-read.c |  7 +++++++
>  examples/glib-main-loop.c   | 14 ++++++++++++--
>  copy/nbd-ops.c              |  4 ++--
>  3 files changed, 21 insertions(+), 4 deletions(-)
>