[Spice-devel] [PATCH] fixup! DCC: change how fill_bits() marshalls data by reference

[Frediano Ziglio]

> 
> This is a possible fix for Frediano's crash. We discussed this on IRC a
> bit and I could not actually reproduce this issue. But I have a possible
> theory about why the drawable may be being kept alive longer than
> expected. In the past, we unreffed the pipe item directly in
> red_channel_client_on_out_msg_done(). Now we rely on the marshaller to
> free/unref the data, but this only happens when spice_marshaller_reset()
> is called. At the moment, this function is not called until right before
> the next message is sent (in red_channel_client_send_item()). To make
> sure that the data is cleaned up after being sent, I've added a reset
> call to _on_out_msg_done() as well.

It works!
However I would put this as separate 1/XX patch. I think the rationale
of 1/9 depends on this patch to be present.

> ---
>  server/red-channel-client.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index 23c9ad4..01bfe17 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -547,13 +547,12 @@ static void
> red_channel_client_restore_main_sender(RedChannelClient *rcc)
>      rcc->priv->send_data.header.data =
>      rcc->priv->send_data.main.header_data;
>  }
>  
> +static void red_channel_client_clear_sent_item(RedChannelClient *rcc);

I prefer at the beginning of the source file.

>  void red_channel_client_on_out_msg_done(void *opaque)
>  {
>      RedChannelClient *rcc = RED_CHANNEL_CLIENT(opaque);
>      int fd;
>  
> -    rcc->priv->send_data.size = 0;
> -
>      if (spice_marshaller_get_fd(rcc->priv->send_data.marshaller, &fd)) {
>          if (reds_stream_send_msgfd(rcc->priv->stream, fd) < 0) {
>              perror("sendfd");
> @@ -578,6 +577,7 @@ void red_channel_client_on_out_msg_done(void *opaque)
>          spice_assert(rcc->priv->send_data.header.data != NULL);
>          red_channel_client_begin_send_message(rcc);
>      } else {
> +        red_channel_client_clear_sent_item(rcc);

why here and not before the urgent check?
I would put before the urgent check if and I would remove
the spice_marshaller_reset from red_channel_client_restore_main_sender.

>          if (rcc->priv->latency_monitor.timer
>              && !rcc->priv->send_data.blocked
>              && g_queue_is_empty(&rcc->priv->pipe)) {
> @@ -1575,6 +1575,7 @@ static void
> red_channel_client_clear_sent_item(RedChannelClient *rcc)
>  {
>      rcc->priv->send_data.blocked = FALSE;
>      rcc->priv->send_data.size = 0;
> +    spice_marshaller_reset(rcc->priv->send_data.marshaller);

This basically replace what red_channel_client_release_sent_item was
doing. I would put it where red_channel_client_release_sent_item was
with a comment explaining the reason we need to reset the marshaller
when the message is just sent. Moved to 1/XX this line would go
after/before  red_channel_client_release_sent_item.

>  }
>  
>  // TODO: again - what is the context exactly? this happens in channel
>  disconnect. but our

Frediano