[Spice-devel,01/11] Remove RedChannel::hold_item callback

Submitted by Frediano Ziglio on May 20, 2016, 1:01 p.m.

Details

Message ID 1463749309-6456-2-git-send-email-fziglio@redhat.com
State New
Headers show
Series "RedChannel refcounting and type safety" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio May 20, 2016, 1:01 p.m.
This is quite confusing and prone to errors.
Use RedPipeItem reference counting instead.
To compensate for the additional reference due to red_pipe_item_ref
in RedChannel sub class with empty hold_item have to add a
red_pipe_item_unref call in send_item.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 server/cursor-channel.c  | 8 --------
 server/display-channel.c | 8 --------
 server/inputs-channel.c  | 6 +-----
 server/main-channel.c    | 8 +-------
 server/red-channel.c     | 4 ++--
 server/red-channel.h     | 2 --
 server/smartcard.c       | 9 ++-------
 server/spicevmc.c        | 8 +-------
 8 files changed, 7 insertions(+), 46 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index fa462c5..104bf51 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -340,13 +340,6 @@  static void cursor_channel_send_item(RedChannelClient *rcc, RedPipeItem *pipe_it
     red_channel_client_begin_send_message(rcc);
 }
 
-static void cursor_channel_hold_pipe_item(RedChannelClient *rcc, RedPipeItem *item)
-{
-    spice_return_if_fail(item);
-
-    red_pipe_item_ref(item);
-}
-
 CursorChannel* cursor_channel_new(RedWorker *worker)
 {
     CursorChannel *cursor_channel;
@@ -354,7 +347,6 @@  CursorChannel* cursor_channel_new(RedWorker *worker)
     ChannelCbs cbs = {
         .on_disconnect =  cursor_channel_client_on_disconnect,
         .send_item = cursor_channel_send_item,
-        .hold_item = cursor_channel_hold_pipe_item,
     };
 
     spice_info("create cursor channel");
diff --git a/server/display-channel.c b/server/display-channel.c
index 3f414fd..b9ed285 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -1930,13 +1930,6 @@  static void send_item(RedChannelClient *rcc, RedPipeItem *item)
     dcc_send_item(RCC_TO_DCC(rcc), item);
 }
 
-static void hold_item(RedChannelClient *rcc, RedPipeItem *item)
-{
-    spice_return_if_fail(item);
-
-    red_pipe_item_ref(item);
-}
-
 static int handle_migrate_flush_mark(RedChannelClient *rcc)
 {
     DisplayChannel *display_channel = SPICE_CONTAINEROF(rcc->channel, DisplayChannel, common.base);
@@ -1977,7 +1970,6 @@  DisplayChannel* display_channel_new(SpiceServer *reds, RedWorker *worker,
     ChannelCbs cbs = {
         .on_disconnect = on_disconnect,
         .send_item = send_item,
-        .hold_item = hold_item,
         .handle_migrate_flush_mark = handle_migrate_flush_mark,
         .handle_migrate_data = handle_migrate_data,
         .handle_migrate_data_get_serial = handle_migrate_data_get_serial
diff --git a/server/inputs-channel.c b/server/inputs-channel.c
index a3c9fb2..3218a48 100644
--- a/server/inputs-channel.c
+++ b/server/inputs-channel.c
@@ -289,6 +289,7 @@  static void inputs_channel_send_item(RedChannelClient *rcc, RedPipeItem *base)
             spice_warning("invalid pipe iten %d", base->type);
             break;
     }
+    red_pipe_item_unref(base);
     red_channel_client_begin_send_message(rcc);
 }
 
@@ -518,10 +519,6 @@  static int inputs_channel_config_socket(RedChannelClient *rcc)
     return TRUE;
 }
 
-static void inputs_channel_hold_pipe_item(RedChannelClient *rcc, RedPipeItem *item)
-{
-}
-
 static void inputs_connect(RedChannel *channel, RedClient *client,
                            RedsStream *stream, int migration,
                            int num_common_caps, uint32_t *common_caps,
@@ -620,7 +617,6 @@  InputsChannel* inputs_channel_new(RedsState *reds)
     channel_cbs.config_socket = inputs_channel_config_socket;
     channel_cbs.on_disconnect = inputs_channel_on_disconnect;
     channel_cbs.send_item = inputs_channel_send_item;
-    channel_cbs.hold_item = inputs_channel_hold_pipe_item;
     channel_cbs.alloc_recv_buf = inputs_channel_alloc_msg_rcv_buf;
     channel_cbs.release_recv_buf = inputs_channel_release_msg_rcv_buf;
     channel_cbs.handle_migrate_data = inputs_channel_handle_migrate_data;
diff --git a/server/main-channel.c b/server/main-channel.c
index d540796..219212c 100644
--- a/server/main-channel.c
+++ b/server/main-channel.c
@@ -788,9 +788,8 @@  static void main_channel_send_item(RedChannelClient *rcc, RedPipeItem *base)
         case RED_PIPE_ITEM_TYPE_MAIN_AGENT_CONNECTED_TOKENS:
             main_channel_marshall_agent_connected(m, rcc, base);
             break;
-        default:
-            break;
     };
+    red_pipe_item_unref(base);
     red_channel_client_begin_send_message(rcc);
 }
 
@@ -1013,10 +1012,6 @@  static int main_channel_config_socket(RedChannelClient *rcc)
     return TRUE;
 }
 
-static void main_channel_hold_pipe_item(RedChannelClient *rcc, RedPipeItem *item)
-{
-}
-
 static int main_channel_handle_migrate_flush_mark(RedChannelClient *rcc)
 {
     spice_debug(NULL);
@@ -1154,7 +1149,6 @@  MainChannel* main_channel_new(RedsState *reds)
     channel_cbs.config_socket = main_channel_config_socket;
     channel_cbs.on_disconnect = main_channel_client_on_disconnect;
     channel_cbs.send_item = main_channel_send_item;
-    channel_cbs.hold_item = main_channel_hold_pipe_item;
     channel_cbs.alloc_recv_buf = main_channel_alloc_msg_rcv_buf;
     channel_cbs.release_recv_buf = main_channel_release_msg_rcv_buf;
     channel_cbs.handle_migrate_flush_mark = main_channel_handle_migrate_flush_mark;
diff --git a/server/red-channel.c b/server/red-channel.c
index 43b053a..8226bc4 100644
--- a/server/red-channel.c
+++ b/server/red-channel.c
@@ -1591,7 +1591,7 @@  void red_channel_client_init_send_data(RedChannelClient *rcc, uint16_t msg_type,
     rcc->send_data.header.set_msg_type(&rcc->send_data.header, msg_type);
     rcc->send_data.item = item;
     if (item) {
-        rcc->channel->channel_cbs.hold_item(rcc, item);
+        red_pipe_item_ref(item);
     }
 }
 
@@ -2374,7 +2374,7 @@  int red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
         end_time = UINT64_MAX;
     }
 
-    rcc->channel->channel_cbs.hold_item(rcc, item);
+    red_pipe_item_ref(item);
 
     if (red_channel_client_blocked(rcc)) {
         red_channel_client_receive(rcc);
diff --git a/server/red-channel.h b/server/red-channel.h
index 63cb2d9..8e8845e 100644
--- a/server/red-channel.h
+++ b/server/red-channel.h
@@ -158,7 +158,6 @@  typedef void (*channel_release_msg_recv_buf_proc)(RedChannelClient *channel,
 typedef void (*channel_disconnect_proc)(RedChannelClient *rcc);
 typedef int (*channel_configure_socket_proc)(RedChannelClient *rcc);
 typedef void (*channel_send_pipe_item_proc)(RedChannelClient *rcc, RedPipeItem *item);
-typedef void (*channel_hold_pipe_item_proc)(RedChannelClient *rcc, RedPipeItem *item);
 typedef void (*channel_on_incoming_error_proc)(RedChannelClient *rcc);
 typedef void (*channel_on_outgoing_error_proc)(RedChannelClient *rcc);
 
@@ -185,7 +184,6 @@  typedef struct {
     channel_configure_socket_proc config_socket;
     channel_disconnect_proc on_disconnect;
     channel_send_pipe_item_proc send_item;
-    channel_hold_pipe_item_proc hold_item;
     channel_alloc_msg_recv_buf_proc alloc_recv_buf;
     channel_release_msg_recv_buf_proc release_recv_buf;
     channel_handle_migrate_flush_mark_proc handle_migrate_flush_mark;
diff --git a/server/smartcard.c b/server/smartcard.c
index a75f01c..e68ccdc 100644
--- a/server/smartcard.c
+++ b/server/smartcard.c
@@ -498,9 +498,10 @@  static void smartcard_channel_send_item(RedChannelClient *rcc, RedPipeItem *item
         break;
     default:
         spice_error("bad pipe item %d", item->type);
-        free(item);
+        red_pipe_item_unref(item);
         return;
     }
+    red_pipe_item_unref(item);
     red_channel_client_begin_send_message(rcc);
 }
 
@@ -737,11 +738,6 @@  static int smartcard_channel_handle_message(RedChannelClient *rcc,
     return TRUE;
 }
 
-static void smartcard_channel_hold_pipe_item(RedChannelClient *rcc,
-                                             RedPipeItem *item)
-{
-}
-
 static void smartcard_connect_client(RedChannel *channel, RedClient *client,
                                      RedsStream *stream, int migration,
                                      int num_common_caps, uint32_t *common_caps,
@@ -785,7 +781,6 @@  static void smartcard_init(RedsState *reds)
     channel_cbs.config_socket = smartcard_channel_client_config_socket;
     channel_cbs.on_disconnect = smartcard_channel_on_disconnect;
     channel_cbs.send_item = smartcard_channel_send_item;
-    channel_cbs.hold_item = smartcard_channel_hold_pipe_item;
     channel_cbs.alloc_recv_buf = smartcard_channel_alloc_msg_rcv_buf;
     channel_cbs.release_recv_buf = smartcard_channel_release_msg_rcv_buf;
     channel_cbs.handle_migrate_flush_mark = smartcard_channel_client_handle_migrate_flush_mark;
diff --git a/server/spicevmc.c b/server/spicevmc.c
index 14d34b4..be51d54 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -355,12 +355,6 @@  static void spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
     }
 }
 
-static void spicevmc_red_channel_hold_pipe_item(RedChannelClient *rcc,
-                                                RedPipeItem *item)
-{
-    /* NOOP */
-}
-
 static void spicevmc_red_channel_send_data(RedChannelClient *rcc,
                                            SpiceMarshaller *m,
                                            RedPipeItem *item)
@@ -434,6 +428,7 @@  static void spicevmc_red_channel_send_item(RedChannelClient *rcc,
         red_pipe_item_unref(item);
         return;
     }
+    red_pipe_item_unref(item);
     red_channel_client_begin_send_message(rcc);
 }
 
@@ -497,7 +492,6 @@  RedCharDevice *spicevmc_device_connect(RedsState *reds,
     channel_cbs.config_socket = spicevmc_red_channel_client_config_socket;
     channel_cbs.on_disconnect = spicevmc_red_channel_client_on_disconnect;
     channel_cbs.send_item = spicevmc_red_channel_send_item;
-    channel_cbs.hold_item = spicevmc_red_channel_hold_pipe_item;
     channel_cbs.alloc_recv_buf = spicevmc_red_channel_alloc_msg_rcv_buf;
     channel_cbs.release_recv_buf = spicevmc_red_channel_release_msg_rcv_buf;
     channel_cbs.handle_migrate_flush_mark = spicevmc_channel_client_handle_migrate_flush_mark;

Comments

On Fri, 2016-05-20 at 14:01 +0100, Frediano Ziglio wrote:
> This is quite confusing and prone to errors.
> Use RedPipeItem reference counting instead.
> To compensate for the additional reference due to red_pipe_item_ref
> in RedChannel sub class with empty hold_item have to add a
> red_pipe_item_unref call in send_item.
> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  server/cursor-channel.c  | 8 --------
>  server/display-channel.c | 8 --------
>  server/inputs-channel.c  | 6 +-----
>  server/main-channel.c    | 8 +-------
>  server/red-channel.c     | 4 ++--
>  server/red-channel.h     | 2 --
>  server/smartcard.c       | 9 ++-------
>  server/spicevmc.c        | 8 +-------
>  8 files changed, 7 insertions(+), 46 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index fa462c5..104bf51 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -340,13 +340,6 @@ static void cursor_channel_send_item(RedChannelClient
> *rcc, RedPipeItem *pipe_it
>      red_channel_client_begin_send_message(rcc);
>  }
>  
> -static void cursor_channel_hold_pipe_item(RedChannelClient *rcc, RedPipeItem
> *item)
> -{
> -    spice_return_if_fail(item);
> -
> -    red_pipe_item_ref(item);
> -}
> -
>  CursorChannel* cursor_channel_new(RedWorker *worker)
>  {
>      CursorChannel *cursor_channel;
> @@ -354,7 +347,6 @@ CursorChannel* cursor_channel_new(RedWorker *worker)
>      ChannelCbs cbs = {
>          .on_disconnect =  cursor_channel_client_on_disconnect,
>          .send_item = cursor_channel_send_item,
> -        .hold_item = cursor_channel_hold_pipe_item,
>      };
>  
>      spice_info("create cursor channel");
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 3f414fd..b9ed285 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1930,13 +1930,6 @@ static void send_item(RedChannelClient *rcc,
> RedPipeItem *item)
>      dcc_send_item(RCC_TO_DCC(rcc), item);
>  }
>  
> -static void hold_item(RedChannelClient *rcc, RedPipeItem *item)
> -{
> -    spice_return_if_fail(item);
> -
> -    red_pipe_item_ref(item);
> -}
> -
>  static int handle_migrate_flush_mark(RedChannelClient *rcc)
>  {
>      DisplayChannel *display_channel = SPICE_CONTAINEROF(rcc->channel,
> DisplayChannel, common.base);
> @@ -1977,7 +1970,6 @@ DisplayChannel* display_channel_new(SpiceServer *reds,
> RedWorker *worker,
>      ChannelCbs cbs = {
>          .on_disconnect = on_disconnect,
>          .send_item = send_item,
> -        .hold_item = hold_item,
>          .handle_migrate_flush_mark = handle_migrate_flush_mark,
>          .handle_migrate_data = handle_migrate_data,
>          .handle_migrate_data_get_serial = handle_migrate_data_get_serial
> diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> index a3c9fb2..3218a48 100644
> --- a/server/inputs-channel.c
> +++ b/server/inputs-channel.c
> @@ -289,6 +289,7 @@ static void inputs_channel_send_item(RedChannelClient
> *rcc, RedPipeItem *base)
>              spice_warning("invalid pipe iten %d", base->type);
>              break;
>      }
> +    red_pipe_item_unref(base);
>      red_channel_client_begin_send_message(rcc);
>  }
>  
> @@ -518,10 +519,6 @@ static int inputs_channel_config_socket(RedChannelClient
> *rcc)
>      return TRUE;
>  }
>  
> -static void inputs_channel_hold_pipe_item(RedChannelClient *rcc, RedPipeItem
> *item)
> -{
> -}
> -
>  static void inputs_connect(RedChannel *channel, RedClient *client,
>                             RedsStream *stream, int migration,
>                             int num_common_caps, uint32_t *common_caps,
> @@ -620,7 +617,6 @@ InputsChannel* inputs_channel_new(RedsState *reds)
>      channel_cbs.config_socket = inputs_channel_config_socket;
>      channel_cbs.on_disconnect = inputs_channel_on_disconnect;
>      channel_cbs.send_item = inputs_channel_send_item;
> -    channel_cbs.hold_item = inputs_channel_hold_pipe_item;
>      channel_cbs.alloc_recv_buf = inputs_channel_alloc_msg_rcv_buf;
>      channel_cbs.release_recv_buf = inputs_channel_release_msg_rcv_buf;
>      channel_cbs.handle_migrate_data = inputs_channel_handle_migrate_data;
> diff --git a/server/main-channel.c b/server/main-channel.c
> index d540796..219212c 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -788,9 +788,8 @@ static void main_channel_send_item(RedChannelClient *rcc,
> RedPipeItem *base)
>          case RED_PIPE_ITEM_TYPE_MAIN_AGENT_CONNECTED_TOKENS:
>              main_channel_marshall_agent_connected(m, rcc, base);
>              break;
> -        default:
> -            break;

this is unrelated to the patch. Consider splitting?

>      };
> +    red_pipe_item_unref(base);
>      red_channel_client_begin_send_message(rcc);
>  }
>  
> @@ -1013,10 +1012,6 @@ static int main_channel_config_socket(RedChannelClient
> *rcc)
>      return TRUE;
>  }
>  
> -static void main_channel_hold_pipe_item(RedChannelClient *rcc, RedPipeItem
> *item)
> -{
> -}
> -
>  static int main_channel_handle_migrate_flush_mark(RedChannelClient *rcc)
>  {
>      spice_debug(NULL);
> @@ -1154,7 +1149,6 @@ MainChannel* main_channel_new(RedsState *reds)
>      channel_cbs.config_socket = main_channel_config_socket;
>      channel_cbs.on_disconnect = main_channel_client_on_disconnect;
>      channel_cbs.send_item = main_channel_send_item;
> -    channel_cbs.hold_item = main_channel_hold_pipe_item;
>      channel_cbs.alloc_recv_buf = main_channel_alloc_msg_rcv_buf;
>      channel_cbs.release_recv_buf = main_channel_release_msg_rcv_buf;
>      channel_cbs.handle_migrate_flush_mark =
> main_channel_handle_migrate_flush_mark;
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 43b053a..8226bc4 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -1591,7 +1591,7 @@ void red_channel_client_init_send_data(RedChannelClient
> *rcc, uint16_t msg_type,
>      rcc->send_data.header.set_msg_type(&rcc->send_data.header, msg_type);
>      rcc->send_data.item = item;
>      if (item) {
> -        rcc->channel->channel_cbs.hold_item(rcc, item);
> +        red_pipe_item_ref(item);
>      }
>  }
>  
> @@ -2374,7 +2374,7 @@ int
> red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>          end_time = UINT64_MAX;
>      }
>  
> -    rcc->channel->channel_cbs.hold_item(rcc, item);
> +    red_pipe_item_ref(item);
>  
>      if (red_channel_client_blocked(rcc)) {
>          red_channel_client_receive(rcc);
> diff --git a/server/red-channel.h b/server/red-channel.h
> index 63cb2d9..8e8845e 100644
> --- a/server/red-channel.h
> +++ b/server/red-channel.h
> @@ -158,7 +158,6 @@ typedef void
> (*channel_release_msg_recv_buf_proc)(RedChannelClient *channel,
>  typedef void (*channel_disconnect_proc)(RedChannelClient *rcc);
>  typedef int (*channel_configure_socket_proc)(RedChannelClient *rcc);
>  typedef void (*channel_send_pipe_item_proc)(RedChannelClient *rcc,
> RedPipeItem *item);
> -typedef void (*channel_hold_pipe_item_proc)(RedChannelClient *rcc,
> RedPipeItem *item);
>  typedef void (*channel_on_incoming_error_proc)(RedChannelClient *rcc);
>  typedef void (*channel_on_outgoing_error_proc)(RedChannelClient *rcc);
>  
> @@ -185,7 +184,6 @@ typedef struct {
>      channel_configure_socket_proc config_socket;
>      channel_disconnect_proc on_disconnect;
>      channel_send_pipe_item_proc send_item;
> -    channel_hold_pipe_item_proc hold_item;
>      channel_alloc_msg_recv_buf_proc alloc_recv_buf;
>      channel_release_msg_recv_buf_proc release_recv_buf;
>      channel_handle_migrate_flush_mark_proc handle_migrate_flush_mark;
> diff --git a/server/smartcard.c b/server/smartcard.c
> index a75f01c..e68ccdc 100644
> --- a/server/smartcard.c
> +++ b/server/smartcard.c
> @@ -498,9 +498,10 @@ static void smartcard_channel_send_item(RedChannelClient
> *rcc, RedPipeItem *item
>          break;
>      default:
>          spice_error("bad pipe item %d", item->type);
> -        free(item);
> +        red_pipe_item_unref(item);
>          return;
>      }
> +    red_pipe_item_unref(item);
>      red_channel_client_begin_send_message(rcc);
>  }
>  
> @@ -737,11 +738,6 @@ static int
> smartcard_channel_handle_message(RedChannelClient *rcc,
>      return TRUE;
>  }
>  
> -static void smartcard_channel_hold_pipe_item(RedChannelClient *rcc,
> -                                             RedPipeItem *item)
> -{
> -}
> -
>  static void smartcard_connect_client(RedChannel *channel, RedClient *client,
>                                       RedsStream *stream, int migration,
>                                       int num_common_caps, uint32_t
> *common_caps,
> @@ -785,7 +781,6 @@ static void smartcard_init(RedsState *reds)
>      channel_cbs.config_socket = smartcard_channel_client_config_socket;
>      channel_cbs.on_disconnect = smartcard_channel_on_disconnect;
>      channel_cbs.send_item = smartcard_channel_send_item;
> -    channel_cbs.hold_item = smartcard_channel_hold_pipe_item;
>      channel_cbs.alloc_recv_buf = smartcard_channel_alloc_msg_rcv_buf;
>      channel_cbs.release_recv_buf = smartcard_channel_release_msg_rcv_buf;
>      channel_cbs.handle_migrate_flush_mark =
> smartcard_channel_client_handle_migrate_flush_mark;
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index 14d34b4..be51d54 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -355,12 +355,6 @@ static void
> spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
>      }
>  }
>  
> -static void spicevmc_red_channel_hold_pipe_item(RedChannelClient *rcc,
> -                                                RedPipeItem *item)
> -{
> -    /* NOOP */
> -}
> -
>  static void spicevmc_red_channel_send_data(RedChannelClient *rcc,
>                                             SpiceMarshaller *m,
>                                             RedPipeItem *item)
> @@ -434,6 +428,7 @@ static void
> spicevmc_red_channel_send_item(RedChannelClient *rcc,
>          red_pipe_item_unref(item);
>          return;
>      }
> +    red_pipe_item_unref(item);
>      red_channel_client_begin_send_message(rcc);
>  }
>  
> @@ -497,7 +492,6 @@ RedCharDevice *spicevmc_device_connect(RedsState *reds,
>      channel_cbs.config_socket = spicevmc_red_channel_client_config_socket;
>      channel_cbs.on_disconnect = spicevmc_red_channel_client_on_disconnect;
>      channel_cbs.send_item = spicevmc_red_channel_send_item;
> -    channel_cbs.hold_item = spicevmc_red_channel_hold_pipe_item;
>      channel_cbs.alloc_recv_buf = spicevmc_red_channel_alloc_msg_rcv_buf;
>      channel_cbs.release_recv_buf = spicevmc_red_channel_release_msg_rcv_buf;
>      channel_cbs.handle_migrate_flush_mark =
> spicevmc_channel_client_handle_migrate_flush_mark;


looks fine otherwise.


Acked-by: Jonathon Jongsma <jjongsma@redhat.com>