Re: [Libguestfs] [libnbd PATCH] lib: Consolidate free callbacks to just happen at retire time

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.

--- 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libguestfs