On 7/13/2023 3:26 PM, Richard W.M. Jones wrote:
On Thu, Jul 13, 2023 at 08:01:09AM -0500, Eric Blake wrote:
On Thu, Jul 13, 2023 at 07:13:37AM -0500, Eric Blake wrote:
I have replaced a call to `nbd_opt_info()` with a call to
`nbd_aio_opt_info()` and passed in a completion callback which just
calls `exit(EXIT_FAILURE)`. So if the completion callback is called
the test should fail, which it doesn't, at least not on my machine.
Isn't that OK? Only .free is required to be called.
For the context callback (for opt_set_meta), .callback may be called
zero, one, or multiple times, but .free should be called exactly once.
But for the completion callback, I agree that the docs state that both
.callback and .free should each be called exactly once ("On the other
hand, if a completion callback is supplied (only possible with
asynchronous commands), it will always be reached exactly once, and
the completion callback must not ignore the value pointed to by
C<error>."); we are indeed missing the call to .callback. I'll work
on a patch.
Eww, the bug is bigger than just nbd_aio_opt* not always calling
completion.callback exactly once. With just this diff (to be folded
into the larger patch I'm working on), I'm getting an assertion
failure that we fail to call completion.callback for
nbd_aio_pread_structured when calling nbd_close() prior to the command
running to completion, so I'll have to fix that too.
Just to be clear about this, are we really sure the completion
callback should always be called once? I'm not clear why that should
be the case. (The .free callback however should be.)
For example, if I call a function with bogus invalid parameters so
that it fails very early on (or when the handle is in an incorrect
state), should I expect the completion callback to be called? I would
expect not.
Rich.
The user needs a way to know if an error occurred. So the completion
callback must be called if the asynchronous function did not fail
(returned 0). If the completion callback should be called, with the
error parameter set, even when the asynchronous function immediately
failed with a non-zero return value is another question. I see two
possibilities: Either the completion callback should always be called.
Or it should be called iff the asynchronous function returned 0 (did not
fail).
By the way, if the error parameter is set in the completion callback,
how can the user retrieve the error text? Is it possible to call
`get_nbd_error(3)` from inside the completion callback?
Best regards,
Tage
diff --git i/tests/closure-lifetimes.c w/tests/closure-lifetimes.c
index 9bb4e120..c5ad4c10 100644
--- i/tests/closure-lifetimes.c
+++ w/tests/closure-lifetimes.c
@@ -66,6 +66,7 @@ read_cb (void *opaque,
uint64_t offset, unsigned status, int *error)
{
assert (!read_cb_freed);
+ assert (!completion_cb_called);
read_cb_called++;
return 0;
}
@@ -73,6 +74,7 @@ read_cb (void *opaque,
static void
read_cb_free (void *opaque)
{
+ assert (completion_cb_called);
read_cb_freed++;
}
@@ -81,6 +83,7 @@ block_status_cb (void *opaque, const char *meta, uint64_t
offset,
uint32_t *entries, size_t nr_entries, int *error)
{
assert (!block_status_cb_freed);
+ assert (!completion_cb_called);
block_status_cb_called++;
return 0;
}
@@ -88,6 +91,7 @@ block_status_cb (void *opaque, const char *meta, uint64_t
offset,
static void
block_status_cb_free (void *opaque)
{
+ assert (completion_cb_called);
block_status_cb_freed++;
}
@@ -102,6 +106,7 @@ completion_cb (void *opaque, int *error)
static void
completion_cb_free (void *opaque)
{
+ assert (completion_cb_called);
completion_cb_freed++;
}
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
