thr3ads.net - Libguestfs - Re: [Libguestfs] [libnbd PATCH] lib: Consolidate free callbacks to just happen at retire time [Aug 2019]

If this information is useful, please help other people find it:
Share via:

Eric Blake

2019-Aug-14 22:38 UTC

[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

Richard W.M. Jones

2019-Aug-15 10:22 UTC

head link

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

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@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

Eric Blake

2019-Aug-15 11:40 UTC

head link

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

On 8/15/19 5:22 AM, Richard W.M. Jones wrote:> 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.
Yeah, having looked at yours, you got rid of the type-punning but added
more convenience macros instead of dropping them; I'm fine with yours.

Good to know we were thinking on the same lines :)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Reasonably Related Threads

Search for more reasonably related threads

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

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

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

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

Reasonably Related Threads