[Spice-devel] [PATCH spice-server 07/10] red-channel-client: Avoid weird memory references using MarkerPipeItem

[Christophe Fergeau]

On Mon, Sep 11, 2017 at 11:15:44AM +0100, Frediano Ziglio wrote:
> Instead of having MarkerPipeItem pointing to a variable on an external
> stuff with the possibility to forget to reset it or having possibly
> dangling pointers use reference counting to keep the item and
> mark the item when sent.

Look good to me, I would have added some more details in the log (that
this stores a reference to an external variable because we need to know
its value after the item has been processed and freed), this makes it
easier to understand why you want reference counting.

Christophe

> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red-channel-client.c | 30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index 34202c492..de3ac27cb 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -218,7 +218,7 @@ typedef struct RedEmptyMsgPipeItem {
>  
>  typedef struct MarkerPipeItem {
>      RedPipeItem base;
> -    gboolean *item_in_pipe;
> +    bool item_in_pipe;
>  } MarkerPipeItem;
>  
>  static void red_channel_client_start_ping_timer(RedChannelClient *rcc, uint32_t timeout)
> @@ -613,6 +613,7 @@ static void red_channel_client_send_item(RedChannelClient *rcc, RedPipeItem *ite
>              red_channel_client_send_ping(rcc);
>              break;
>          case RED_PIPE_ITEM_TYPE_MARKER:
> +            SPICE_UPCAST(MarkerPipeItem, item)->item_in_pipe = false;
>              break;
>          default:
>              red_channel_send_item(rcc->priv->channel, rcc, item);
> @@ -1757,23 +1758,13 @@ void red_channel_client_set_header_sub_list(RedChannelClient *rcc, uint32_t sub_
>      rcc->priv->send_data.header.set_msg_sub_list(&rcc->priv->send_data.header, sub_list);
>  }
>  
> -static void marker_pipe_item_free(RedPipeItem *base)
> -{
> -    MarkerPipeItem *item = SPICE_UPCAST(MarkerPipeItem, base);
> -
> -    if (item->item_in_pipe) {
> -        *item->item_in_pipe = FALSE;
> -    }
> -    free(item);
> -}
> -
>  /* TODO: more evil sync stuff. anything with the word wait in it's name. */
>  bool red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>                                              GList *item_pos,
>                                              int64_t timeout)
>  {
>      uint64_t end_time;
> -    gboolean item_in_pipe;
> +    bool item_in_pipe;
>  
>      spice_debug("trace");
>  
> @@ -1785,10 +1776,9 @@ bool red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>  
>      MarkerPipeItem *mark_item = spice_new0(MarkerPipeItem, 1);
>  
> -    red_pipe_item_init_full(&mark_item->base, RED_PIPE_ITEM_TYPE_MARKER,
> -                            marker_pipe_item_free);
> -    item_in_pipe = TRUE;
> -    mark_item->item_in_pipe = &item_in_pipe;
> +    red_pipe_item_init(&mark_item->base, RED_PIPE_ITEM_TYPE_MARKER);
> +    mark_item->item_in_pipe = true;
> +    red_pipe_item_ref(&mark_item->base);
>      red_channel_client_pipe_add_after_pos(rcc, &mark_item->base, item_pos);
>  
>      if (red_channel_client_is_blocked(rcc)) {
> @@ -1797,7 +1787,7 @@ bool red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>      }
>      red_channel_client_push(rcc);
>  
> -    while (item_in_pipe &&
> +    while (mark_item->item_in_pipe &&
>             (timeout == -1 || spice_get_monotonic_time_ns() < end_time)) {
>          usleep(CHANNEL_BLOCKED_SLEEP_DURATION);
>          red_channel_client_receive(rcc);
> @@ -1805,9 +1795,11 @@ bool red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>          red_channel_client_push(rcc);
>      }
>  
> +    item_in_pipe = mark_item->item_in_pipe;
> +    red_pipe_item_unref(&mark_item->base);
> +
>      if (item_in_pipe) {
> -        // still on the queue, make sure won't overwrite the stack variable
> -        mark_item->item_in_pipe = NULL;
> +        // still on the queue
>          spice_warning("timeout");
>          return FALSE;
>      } else {
> -- 
> 2.13.5
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel