[Libguestfs] [libnbd PATCH] lib: Consolidate free callbacks to just happen at retire time
Richard W.M. Jones
rjones at redhat.com
Thu Aug 15 10:22:43 UTC 2019
On Wed, Aug 14, 2019 at 05:38:31PM -0500, Eric Blake wrote:
> When we introduced valid_flags, there was an incentive to do as few
> callbacks as possible, favoring cb(VALID|FREE) calls over the sequence
> cb(VALID);cb(FREE). To make it work, we set .callback=NULL after an
> early free, so that the later check during retirement didn't free
> again.
>
> But now that our .free callback is distinct from our other callbacks,
> there is no longer an advantage to bundling things, and a significant
> reduction in lines of code by just doing it in one place. This
> basically reverts 9c8fccdf (which had slightly-questionable C
> type-punning anyway) and 57150880.
I didn't see this one before implementing an alternate version here:
https://www.redhat.com/archives/libguestfs/2019-August/msg00251.html
https://www.redhat.com/archives/libguestfs/2019-August/msg00253.html
They're fairly similar, but I didn't get rid of the FREE_CALLBACK
macro for reasons which are clearer later in the series.
Rich.
> ---
> lib/internal.h | 14 --------------
> generator/states-reply-simple.c | 1 -
> generator/states-reply-structured.c | 20 +-------------------
> generator/states-reply.c | 1 -
> generator/states.c | 1 -
> lib/aio.c | 11 ++++++-----
> lib/debug.c | 4 +++-
> 7 files changed, 10 insertions(+), 42 deletions(-)
>
> diff --git a/lib/internal.h b/lib/internal.h
> index dc3676a..1971613 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -277,20 +277,6 @@ struct command {
> #define CALL_CALLBACK(cb, ...) \
> (cb).callback ((cb).user_data, ##__VA_ARGS__)
>
> -/* Free a callback.
> - *
> - * Note this works for any type of callback because the basic layout
> - * of the struct is the same for all of them. Therefore casting cb to
> - * nbd_completion_callback does not change the effective code.
> - */
> -#define FREE_CALLBACK(cb) \
> - do { \
> - nbd_completion_callback *_cb = (nbd_completion_callback *)&(cb); \
> - if (_cb->callback != NULL && _cb->free != NULL) \
> - _cb->free (_cb->user_data); \
> - _cb->callback = NULL; \
> - } while (0)
> -
> /* aio.c */
> extern void nbd_internal_retire_and_free_command (struct command *);
>
> diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
> index 8e3d7f1..2f83e6f 100644
> --- a/generator/states-reply-simple.c
> +++ b/generator/states-reply-simple.c
> @@ -69,7 +69,6 @@
> cmd->offset, LIBNBD_READ_DATA,
> &error) == -1)
> cmd->error = error ? error : EPROTO;
> - FREE_CALLBACK (cmd->cb.fn.chunk);
> }
>
> SET_NEXT_STATE (%^FINISH_COMMAND);
> diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c
> index 2e327ce..58e83d4 100644
> --- a/generator/states-reply-structured.c
> +++ b/generator/states-reply-structured.c
> @@ -295,7 +295,6 @@
> }
> if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.callback) {
> int scratch = error;
> - uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
>
> /* Different from successful reads: inform the callback about the
> * current error rather than any earlier one. If the callback fails
> @@ -307,8 +306,6 @@
> &scratch) == -1)
> if (cmd->error == 0)
> cmd->error = scratch;
> - if (flags & NBD_REPLY_FLAG_DONE)
> - FREE_CALLBACK (cmd->cb.fn.chunk);
> }
> }
>
> @@ -390,15 +387,12 @@
> assert (cmd); /* guaranteed by CHECK */
> if (cmd->cb.fn.chunk.callback) {
> int error = cmd->error;
> - uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
>
> if (CALL_CALLBACK (cmd->cb.fn.chunk, cmd->data + (offset - cmd->offset),
> length - sizeof offset, offset,
> LIBNBD_READ_DATA, &error) == -1)
> if (cmd->error == 0)
> cmd->error = error ? error : EPROTO;
> - if (flags & NBD_REPLY_FLAG_DONE)
> - FREE_CALLBACK (cmd->cb.fn.chunk);
> }
>
> SET_NEXT_STATE (%FINISH);
> @@ -454,7 +448,6 @@
> memset (cmd->data + offset, 0, length);
> if (cmd->cb.fn.chunk.callback) {
> int error = cmd->error;
> - uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
>
> if (CALL_CALLBACK (cmd->cb.fn.chunk,
> cmd->data + offset, length,
> @@ -462,8 +455,6 @@
> LIBNBD_READ_HOLE, &error) == -1)
> if (cmd->error == 0)
> cmd->error = error ? error : EPROTO;
> - if (flags & NBD_REPLY_FLAG_DONE)
> - FREE_CALLBACK (cmd->cb.fn.chunk);
> }
>
> SET_NEXT_STATE(%FINISH);
> @@ -509,7 +500,6 @@
> if (meta_context) {
> /* Call the caller's extent function. */
> int error = cmd->error;
> - uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
>
> if (CALL_CALLBACK (cmd->cb.fn.extent,
> meta_context->name, cmd->offset,
> @@ -517,8 +507,6 @@
> &error) == -1)
> if (cmd->error == 0)
> cmd->error = error ? error : EPROTO;
> - if (flags & NBD_REPLY_FLAG_DONE)
> - FREE_CALLBACK (cmd->cb.fn.extent);
> }
> else
> /* Emit a debug message, but ignore it. */
> @@ -530,17 +518,11 @@
> return 0;
>
> REPLY.STRUCTURED_REPLY.FINISH:
> - struct command *cmd = h->reply_cmd;
> uint16_t flags;
>
> flags = be16toh (h->sbuf.sr.structured_reply.flags);
> - if (flags & NBD_REPLY_FLAG_DONE) {
> - if (cmd->type == NBD_CMD_BLOCK_STATUS)
> - FREE_CALLBACK (cmd->cb.fn.extent);
> - if (cmd->type == NBD_CMD_READ)
> - FREE_CALLBACK (cmd->cb.fn.chunk);
> + if (flags & NBD_REPLY_FLAG_DONE)
> SET_NEXT_STATE (%^FINISH_COMMAND);
> - }
> else {
> h->reply_cmd = NULL;
> SET_NEXT_STATE (%.READY);
> diff --git a/generator/states-reply.c b/generator/states-reply.c
> index 41dcf8d..575a6d1 100644
> --- a/generator/states-reply.c
> +++ b/generator/states-reply.c
> @@ -174,7 +174,6 @@ save_reply_state (struct nbd_handle *h)
>
> assert (cmd->type != NBD_CMD_DISC);
> r = CALL_CALLBACK (cmd->cb.completion, &error);
> - FREE_CALLBACK (cmd->cb.completion);
> switch (r) {
> case -1:
> if (error)
> diff --git a/generator/states.c b/generator/states.c
> index 263f60e..cfa9957 100644
> --- a/generator/states.c
> +++ b/generator/states.c
> @@ -127,7 +127,6 @@ void abort_commands (struct nbd_handle *h,
>
> assert (cmd->type != NBD_CMD_DISC);
> r = CALL_CALLBACK (cmd->cb.completion, &error);
> - FREE_CALLBACK (cmd->cb.completion);
> switch (r) {
> case -1:
> if (error)
> diff --git a/lib/aio.c b/lib/aio.c
> index 38a27d0..4a219e4 100644
> --- a/lib/aio.c
> +++ b/lib/aio.c
> @@ -32,11 +32,12 @@ void
> nbd_internal_retire_and_free_command (struct command *cmd)
> {
> /* Free the callbacks. */
> - if (cmd->type == NBD_CMD_BLOCK_STATUS)
> - FREE_CALLBACK (cmd->cb.fn.extent);
> - if (cmd->type == NBD_CMD_READ)
> - FREE_CALLBACK (cmd->cb.fn.chunk);
> - FREE_CALLBACK (cmd->cb.completion);
> + if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent.free)
> + cmd->cb.fn.extent.free (cmd->cb.fn.extent.user_data);
> + if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.free)
> + cmd->cb.fn.chunk.free (cmd->cb.fn.chunk.user_data);
> + if (cmd->cb.completion.free)
> + cmd->cb.completion.free (cmd->cb.completion.user_data);
>
> free (cmd);
> }
> diff --git a/lib/debug.c b/lib/debug.c
> index eec2051..a0e6636 100644
> --- a/lib/debug.c
> +++ b/lib/debug.c
> @@ -41,7 +41,9 @@ nbd_unlocked_get_debug (struct nbd_handle *h)
> int
> nbd_unlocked_clear_debug_callback (struct nbd_handle *h)
> {
> - FREE_CALLBACK (h->debug_callback);
> + if (h->debug_callback.free)
> + h->debug_callback.free (h->debug_callback.user_data);
> + memset (&h->debug_callback, 0, sizeof h->debug_callback);
> return 0;
> }
>
> --
> 2.20.1
>
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
More information about the Libguestfs
mailing list