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

[Victor Toso]

Hi,

As always, many thanks for your thorough review.

On Thu, Jul 13, 2017 at 11:43:29AM -0500, Jonathon Jongsma wrote:
> 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?

The current check is the only check that we do, in this patch:

  if (stream_id < c->nstreams) {
      /* No need to increase c->streams */
      return;
  }

and previously:

  if (op->id >= c->nstreams) {
    ...
  }

So, the following part of the comment...

 "As the id itself is not enforced by the protocol, we should keep the
  current check to avoid regressions or other unknown issues."

... is only related to the above check and that we should not remove
otherwise it could cause regressions in my point of view.

> > + */
> > +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;
> }

s/func/channel_member_prepend/

>
> 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() */
> }

The hint should be the function name... s/init/channel_member_prepend()/

> 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

The purpose of this patch was more about documentation. Even if this is
called just once, from the check mentioned above with op->id, it's not
easy to me to understand when the array is allocated or not.

The function name and the comments had the only purpose to make this
easier to follow, not harder. If that's how you feel about it, we can
ignore the patches.

Cheers,
    toso
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170714/cec95502/attachment.sig>