[spice-gtk,v1,4/4] channel-display: Limit number of streams that can be created

Submitted by Victor Toso on April 19, 2018, 6:40 a.m.

Details

Message ID 20180419064041.8925-5-victortoso@redhat.com
State New
Headers show
Series "Add a max-num-streams in protocol" ( rev: 2 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso April 19, 2018, 6:40 a.m.
From: Victor Toso <me@victortoso.com>

Using the limit SPICE_MAX_NUM_STREAMS introduced in spice-protocol
0.12.14 to fix the amount of streams we might be working currently.

With this change, let's create the streams array on _init() and free
it on _finalize() to avoid dealing with too many checks everywhere.

Note that this patch removes the nstreams as the array size is fixed.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 configure.ac          |  2 +-
 src/channel-display.c | 28 +++++++---------------------
 2 files changed, 8 insertions(+), 22 deletions(-)

Patch hide | download patch | download mbox

diff --git a/configure.ac b/configure.ac
index a9a7eb9..768e180 100644
--- a/configure.ac
+++ b/configure.ac
@@ -86,7 +86,7 @@  AC_CHECK_LIBM
 AC_SUBST(LIBM)
 
 AC_CONFIG_SUBDIRS([spice-common])
-PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= 0.12.13])
+PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= 0.12.14])
 
 COMMON_CFLAGS='-I${top_builddir}/spice-common/ -I${top_srcdir}/spice-common/ ${SPICE_PROTOCOL_CFLAGS}'
 AC_SUBST(COMMON_CFLAGS)
diff --git a/src/channel-display.c b/src/channel-display.c
index 5329ed0..50c896d 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -64,7 +64,6 @@  struct _SpiceDisplayChannelPrivate {
     SpiceImageSurfaces          image_surfaces;
     SpiceGlzDecoderWindow       *glz_window;
     display_stream              **streams;
-    int                         nstreams;
     gboolean                    mark;
     guint                       mark_false_event_id;
     GArray                      *monitors;
@@ -169,6 +168,7 @@  static void spice_display_channel_finalize(GObject *object)
     clear_surfaces(SPICE_CHANNEL(object), FALSE);
     g_hash_table_unref(c->surfaces);
     clear_streams(SPICE_CHANNEL(object));
+    g_clear_pointer(&c->streams, g_free);
     g_clear_pointer(&c->palettes, cache_free);
 
     if (G_OBJECT_CLASS(spice_display_channel_parent_class)->finalize)
@@ -862,6 +862,7 @@  static void spice_display_channel_init(SpiceDisplayChannel *channel)
 
     c = channel->priv = SPICE_DISPLAY_CHANNEL_GET_PRIVATE(channel);
 
+    c->streams = g_new0(display_stream *, SPICE_MAX_NUM_STREAMS);
     c->surfaces = g_hash_table_new_full(NULL, NULL, NULL, destroy_surface);
     c->image_cache.ops = &image_cache_ops;
     c->palette_cache.ops = &palette_cache_ops;
@@ -1209,8 +1210,7 @@  static display_stream *get_stream_by_id(SpiceChannel *channel, uint32_t id)
 {
     SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
 
-    if (c != NULL && c->streams != NULL && id < c->nstreams &&
-        c->streams[id] != NULL) {
+    if (c != NULL && id < SPICE_MAX_NUM_STREAMS && c->streams[id] != NULL) {
         return c->streams[id];
     }
 
@@ -1263,7 +1263,7 @@  static void destroy_stream(SpiceChannel *channel, int id)
 
     g_return_if_fail(c != NULL);
     g_return_if_fail(c->streams != NULL);
-    g_return_if_fail(c->nstreams > id);
+    g_return_if_fail(id < SPICE_MAX_NUM_STREAMS);
 
     g_clear_pointer(&c->streams[id], display_stream_destroy);
 }
@@ -1274,18 +1274,7 @@  static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
     SpiceMsgDisplayStreamCreate *op = spice_msg_in_parsed(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]));
-    }
+    g_return_if_fail(op->id < SPICE_MAX_NUM_STREAMS);
     g_return_if_fail(c->streams[op->id] == NULL);
 
     c->streams[op->id] = display_stream_create(channel, op->id, op->surface_id,
@@ -1474,7 +1463,7 @@  static void display_session_mm_time_reset_cb(SpiceSession *session, gpointer dat
 
     CHANNEL_DEBUG(channel, "%s", __FUNCTION__);
 
-    for (i = 0; i < c->nstreams; i++) {
+    for (i = 0; i < SPICE_MAX_NUM_STREAMS; i++) {
         display_stream *st;
 
         if (c->streams[i] == NULL) {
@@ -1625,14 +1614,11 @@  static void display_stream_destroy(gpointer st_pointer)
 
 static void clear_streams(SpiceChannel *channel)
 {
-    SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
     int i;
 
-    for (i = 0; i < c->nstreams; i++) {
+    for (i = 0; i < SPICE_MAX_NUM_STREAMS; i++) {
         destroy_stream(channel, i);
     }
-    g_clear_pointer(&c->streams, g_free);
-    c->nstreams = 0;
 }
 
 /* coroutine context */

Comments

> 
> From: Victor Toso <me@victortoso.com>
> 
> Using the limit SPICE_MAX_NUM_STREAMS introduced in spice-protocol
> 0.12.14 to fix the amount of streams we might be working currently.
> 
> With this change, let's create the streams array on _init() and free
> it on _finalize() to avoid dealing with too many checks everywhere.
> 
> Note that this patch removes the nstreams as the array size is fixed.
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  configure.ac          |  2 +-
>  src/channel-display.c | 28 +++++++---------------------
>  2 files changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index a9a7eb9..768e180 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -86,7 +86,7 @@ AC_CHECK_LIBM
>  AC_SUBST(LIBM)
>  
>  AC_CONFIG_SUBDIRS([spice-common])
> -PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= 0.12.13])
> +PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= 0.12.14])
>  
>  COMMON_CFLAGS='-I${top_builddir}/spice-common/ -I${top_srcdir}/spice-common/
>  ${SPICE_PROTOCOL_CFLAGS}'
>  AC_SUBST(COMMON_CFLAGS)
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 5329ed0..50c896d 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -64,7 +64,6 @@ struct _SpiceDisplayChannelPrivate {
>      SpiceImageSurfaces          image_surfaces;
>      SpiceGlzDecoderWindow       *glz_window;
>      display_stream              **streams;
> -    int                         nstreams;
>      gboolean                    mark;
>      guint                       mark_false_event_id;
>      GArray                      *monitors;
> @@ -169,6 +168,7 @@ static void spice_display_channel_finalize(GObject
> *object)
>      clear_surfaces(SPICE_CHANNEL(object), FALSE);
>      g_hash_table_unref(c->surfaces);
>      clear_streams(SPICE_CHANNEL(object));
> +    g_clear_pointer(&c->streams, g_free);
>      g_clear_pointer(&c->palettes, cache_free);
>  
>      if (G_OBJECT_CLASS(spice_display_channel_parent_class)->finalize)
> @@ -862,6 +862,7 @@ static void
> spice_display_channel_init(SpiceDisplayChannel *channel)
>  
>      c = channel->priv = SPICE_DISPLAY_CHANNEL_GET_PRIVATE(channel);
>  
> +    c->streams = g_new0(display_stream *, SPICE_MAX_NUM_STREAMS);
>      c->surfaces = g_hash_table_new_full(NULL, NULL, NULL, destroy_surface);
>      c->image_cache.ops = &image_cache_ops;
>      c->palette_cache.ops = &palette_cache_ops;
> @@ -1209,8 +1210,7 @@ static display_stream *get_stream_by_id(SpiceChannel
> *channel, uint32_t id)
>  {
>      SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
>  
> -    if (c != NULL && c->streams != NULL && id < c->nstreams &&
> -        c->streams[id] != NULL) {
> +    if (c != NULL && id < SPICE_MAX_NUM_STREAMS && c->streams[id] != NULL) {
>          return c->streams[id];
>      }
>  
> @@ -1263,7 +1263,7 @@ static void destroy_stream(SpiceChannel *channel, int
> id)
>  
>      g_return_if_fail(c != NULL);
>      g_return_if_fail(c->streams != NULL);
> -    g_return_if_fail(c->nstreams > id);
> +    g_return_if_fail(id < SPICE_MAX_NUM_STREAMS);

As int could potentially also be <0, maybe better change these IDs to make sure
there are unsigned?

>  
>      g_clear_pointer(&c->streams[id], display_stream_destroy);
>  }
> @@ -1274,18 +1274,7 @@ static void display_handle_stream_create(SpiceChannel
> *channel, SpiceMsgIn *in)
>      SpiceMsgDisplayStreamCreate *op = spice_msg_in_parsed(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]));
> -    }
> +    g_return_if_fail(op->id < SPICE_MAX_NUM_STREAMS);
>      g_return_if_fail(c->streams[op->id] == NULL);
>  
>      c->streams[op->id] = display_stream_create(channel, op->id,
>      op->surface_id,
> @@ -1474,7 +1463,7 @@ static void
> display_session_mm_time_reset_cb(SpiceSession *session, gpointer dat
>  
>      CHANNEL_DEBUG(channel, "%s", __FUNCTION__);
>  
> -    for (i = 0; i < c->nstreams; i++) {
> +    for (i = 0; i < SPICE_MAX_NUM_STREAMS; i++) {
>          display_stream *st;
>  
>          if (c->streams[i] == NULL) {
> @@ -1625,14 +1614,11 @@ static void display_stream_destroy(gpointer
> st_pointer)
>  
>  static void clear_streams(SpiceChannel *channel)
>  {
> -    SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
>      int i;
>  
> -    for (i = 0; i < c->nstreams; i++) {
> +    for (i = 0; i < SPICE_MAX_NUM_STREAMS; i++) {
>          destroy_stream(channel, i);
>      }
> -    g_clear_pointer(&c->streams, g_free);
> -    c->nstreams = 0;
>  }
>  
>  /* coroutine context */

Frediano
Hi,

On Thu, Apr 19, 2018 at 03:29:00AM -0400, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me@victortoso.com>
> > 
> > Using the limit SPICE_MAX_NUM_STREAMS introduced in spice-protocol
> > 0.12.14 to fix the amount of streams we might be working currently.
> > 
> > With this change, let's create the streams array on _init() and free
> > it on _finalize() to avoid dealing with too many checks everywhere.
> > 
> > Note that this patch removes the nstreams as the array size is fixed.
> > 
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  configure.ac          |  2 +-
> >  src/channel-display.c | 28 +++++++---------------------
> >  2 files changed, 8 insertions(+), 22 deletions(-)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index a9a7eb9..768e180 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -86,7 +86,7 @@ AC_CHECK_LIBM
> >  AC_SUBST(LIBM)
> >  
> >  AC_CONFIG_SUBDIRS([spice-common])
> > -PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= 0.12.13])
> > +PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= 0.12.14])
> >  
> >  COMMON_CFLAGS='-I${top_builddir}/spice-common/ -I${top_srcdir}/spice-common/
> >  ${SPICE_PROTOCOL_CFLAGS}'
> >  AC_SUBST(COMMON_CFLAGS)
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index 5329ed0..50c896d 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -64,7 +64,6 @@ struct _SpiceDisplayChannelPrivate {
> >      SpiceImageSurfaces          image_surfaces;
> >      SpiceGlzDecoderWindow       *glz_window;
> >      display_stream              **streams;
> > -    int                         nstreams;
> >      gboolean                    mark;
> >      guint                       mark_false_event_id;
> >      GArray                      *monitors;
> > @@ -169,6 +168,7 @@ static void spice_display_channel_finalize(GObject
> > *object)
> >      clear_surfaces(SPICE_CHANNEL(object), FALSE);
> >      g_hash_table_unref(c->surfaces);
> >      clear_streams(SPICE_CHANNEL(object));
> > +    g_clear_pointer(&c->streams, g_free);
> >      g_clear_pointer(&c->palettes, cache_free);
> >  
> >      if (G_OBJECT_CLASS(spice_display_channel_parent_class)->finalize)
> > @@ -862,6 +862,7 @@ static void
> > spice_display_channel_init(SpiceDisplayChannel *channel)
> >  
> >      c = channel->priv = SPICE_DISPLAY_CHANNEL_GET_PRIVATE(channel);
> >  
> > +    c->streams = g_new0(display_stream *, SPICE_MAX_NUM_STREAMS);
> >      c->surfaces = g_hash_table_new_full(NULL, NULL, NULL, destroy_surface);
> >      c->image_cache.ops = &image_cache_ops;
> >      c->palette_cache.ops = &palette_cache_ops;
> > @@ -1209,8 +1210,7 @@ static display_stream *get_stream_by_id(SpiceChannel
> > *channel, uint32_t id)
> >  {
> >      SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
> >  
> > -    if (c != NULL && c->streams != NULL && id < c->nstreams &&
> > -        c->streams[id] != NULL) {
> > +    if (c != NULL && id < SPICE_MAX_NUM_STREAMS && c->streams[id] != NULL) {
> >          return c->streams[id];
> >      }
> >  
> > @@ -1263,7 +1263,7 @@ static void destroy_stream(SpiceChannel *channel, int
> > id)
> >  
> >      g_return_if_fail(c != NULL);
> >      g_return_if_fail(c->streams != NULL);
> > -    g_return_if_fail(c->nstreams > id);
> > +    g_return_if_fail(id < SPICE_MAX_NUM_STREAMS);
> 
> As int could potentially also be <0, maybe better change these
> IDs to make sure there are unsigned?

Oh, well, yes. Let me do another patch for that, I'll check if
something else might need to be changed too.

Cheers,
        toso


> 
> >  
> >      g_clear_pointer(&c->streams[id], display_stream_destroy);
> >  }
> > @@ -1274,18 +1274,7 @@ static void display_handle_stream_create(SpiceChannel
> > *channel, SpiceMsgIn *in)
> >      SpiceMsgDisplayStreamCreate *op = spice_msg_in_parsed(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]));
> > -    }
> > +    g_return_if_fail(op->id < SPICE_MAX_NUM_STREAMS);
> >      g_return_if_fail(c->streams[op->id] == NULL);
> >  
> >      c->streams[op->id] = display_stream_create(channel, op->id,
> >      op->surface_id,
> > @@ -1474,7 +1463,7 @@ static void
> > display_session_mm_time_reset_cb(SpiceSession *session, gpointer dat
> >  
> >      CHANNEL_DEBUG(channel, "%s", __FUNCTION__);
> >  
> > -    for (i = 0; i < c->nstreams; i++) {
> > +    for (i = 0; i < SPICE_MAX_NUM_STREAMS; i++) {
> >          display_stream *st;
> >  
> >          if (c->streams[i] == NULL) {
> > @@ -1625,14 +1614,11 @@ static void display_stream_destroy(gpointer
> > st_pointer)
> >  
> >  static void clear_streams(SpiceChannel *channel)
> >  {
> > -    SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
> >      int i;
> >  
> > -    for (i = 0; i < c->nstreams; i++) {
> > +    for (i = 0; i < SPICE_MAX_NUM_STREAMS; i++) {
> >          destroy_stream(channel, i);
> >      }
> > -    g_clear_pointer(&c->streams, g_free);
> > -    c->nstreams = 0;
> >  }
> >  
> >  /* coroutine context */
> 
> Frediano