On Fri, 2017-07-14 at 11:51 +0200, Victor Toso wrote:
> 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@xxxxxxxxxxxxxx>
> > >
> > > Including some comment about current implementation of stream-id
> > > value
> > > in the Spice.
> > >
> > > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx>
> > > ---
> > > 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.
OK, so after a short discussion on IRC, I think I understand a bit
better. Possible comment sugggestion:
"Don't make any assumptions about the order in which the server sends
stream IDs. Currently the spice server implementation sends higher IDs
first and decrements the ID for each subsequent stream, but since the
protocol doesn't impose any requirements about the stream ID, we should
be able to handle other scenarios as well."
Is that sort of what you were trying to convey? Or does that make it
more confusing?
>
> > > + */
> > > +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/
I intentionally chose a non-descriptive function name to exaggerate the
issue ;) More below.
>
> >
> > 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()/
Yes, the function name helps give a hint, but I still like to avoid
side-effects as much as possible. I think it generally makes code more
robust to not rely on them. I'm not dogmatic about it, but I like to
try to find better approaches when I can. For example, consider the
difference between these two approaches:
void init_bar(Foo *foo, guint id);
void func(Foo *foo)
{
...
init_bar(foo, id);
// the caller relies on the fact that the above function allocated
// enough space for the member and assigned it to foo->priv->bar.
foo->priv->bar[id] = ...;
}
vs. something like:
Bar** init_bar(Foo *foo, guint id);
void func(Foo *foo)
{
...
foo->priv->bar = init_bar(foo, id);
// here the caller doesn't have to make assumptions about the
// helper function. Since this function is the one that relies on
// foo->priv->bar being assigned, it keeps that responsibility
// within this function.
foo->priv->bar[id] = ...;
}
Obviously, this is not exactly the same situation as your patch but it
illustrates my point, I think. It's not so much the helper function
that I was objecting to -- it's the fact that we're relying on side-
effects of the helper function. But I don't feel too strongly about
it.
>
> > 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel