[Spice-devel] [PATCH spice-gtk 1/2] display: factor out initialization of stream array

[Jonathon Jongsma]

On Fri, 2017-06-30 at 12:51 +0200, Victor Toso wrote:
> From: Victor Toso <me at victortoso.com>
> 
> Including some comment about current implementation of stream-id
> value
> in the Spice.
> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
>  src/channel-display.c | 42 +++++++++++++++++++++++++++++++--------
> ---
>  1 file changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 06c503c..9ae2851 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -105,6 +105,7 @@ static void
> channel_set_handlers(SpiceChannelClass *klass);
>  
>  static void clear_surfaces(SpiceChannel *channel, gboolean
> keep_primary);
>  static void clear_streams(SpiceChannel *channel);
> +static void streams_check_init(SpiceChannel *channel, guint
> stream_id);
>  static display_surface *find_surface(SpiceDisplayChannelPrivate *c,
> guint32 surface_id);
>  static void spice_display_channel_reset(SpiceChannel *channel,
> gboolean migrating);
>  static void spice_display_channel_reset_capabilities(SpiceChannel
> *channel);
> @@ -1233,17 +1234,7 @@ static void
> display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
>  
>      CHANNEL_DEBUG(channel, "%s: id %u", __FUNCTION__, op->id);
>  
> -    if (op->id >= c->nstreams) {
> -        int n = c->nstreams;
> -        if (!c->nstreams) {
> -            c->nstreams = 1;
> -        }
> -        while (op->id >= c->nstreams) {
> -            c->nstreams *= 2;
> -        }
> -        c->streams = realloc(c->streams, c->nstreams * sizeof(c-
> >streams[0]));
> -        memset(c->streams + n, 0, (c->nstreams - n) * sizeof(c-
> >streams[0]));
> -    }
> +    streams_check_init(channel, op->id);
>      g_return_if_fail(c->streams[op->id] == NULL);
>  
>      c->streams[op->id] = display_stream_create(channel, op-
> >surface_id,
> @@ -1593,6 +1584,35 @@ static void clear_streams(SpiceChannel
> *channel)
>      c->nstreams = 0;
>  }
>  
> +/* Always called on SPICE_MSG_DISPLAY_STREAM_CREATE to verify if the
> c->streams
> + * array is big enough to handle the new stream. This obviously
> takes in
> + * consideration that the stream_id could grow overtime from 0 to
> MAX_STREAMS
> + * value but on server side, the id is picked in descending order,
> starting with
> + * (MAX_STREAMS - 1). As the id itself is not enforced by the
> protocol, we
> + * should keep the current check to avoid regressions or other
> unknown issues.

This comment is not totally clear to me. What is the "current check"
that you're referring to here?


> + */
> +static void streams_check_init(SpiceChannel *channel, guint
> stream_id)
> +{
> +    SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)-
> >priv;
> +    gint old, new;
> +
> +    if (stream_id < c->nstreams) {
> +        /* No need to increase c->streams */
> +        return;
> +    }
> +
> +    old = c->nstreams;
> +    new = (c->nstreams == 0) ? 1 : c->nstreams;
> +
> +    while (stream_id > new) {
> +        new *= 2;
> +    }
> +
> +    c->nstreams = new;
> +    c->streams = realloc(c->streams, new * sizeof(c->streams[0]));
> +    memset(c->streams + old, 0, (new - old) * sizeof(c-
> >streams[0]));
> +}
> +
>  /* coroutine context */
>  static void display_handle_stream_destroy(SpiceChannel *channel,
> SpiceMsgIn *in)
>  {



To be honest, I don't know if I see the value in factoring this out to
a separate function. It's only called from a single place, and in some
ways it makes the behavior of the code a bit more opaque since the
caller is relying on side-effects of the called function. For example,
consider the following simplified and contrived example. I think this
code is pretty clear:

void func(SpiceChannel *channel, uint8_t n)
{
  if (!channel->priv->member) {
    channel->priv->member = g_new0(uint8_t, NUM_ELEMENTS);
  }
  channel->priv->member[0] = n;
}



This code is less clear:

void func(SpiceChannel *channel, uint8_t n)
{
  init(channel);
  channel->priv->member[0] = n;
  /* how do we know that the priv->member array has been allocated?
   * we have to go look at the implementation of init() */
}

void init(SpiceChannel *channel)
{
  if (!channel->priv->member) {
    channel->priv->member = g_new0(uint8_t, NUM_ELEMENTS);
  }
}


If the same init() code was used multiple places, it would make more
sense to factor it out to a separate function, but I'm not sure it's
very beneficial when it's just called from one location.

Jonathon