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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel