Re: [Libguestfs] [libnbd PATCH 0/4] Work towards fixing nbdcopy silent corruption
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(-)