Re: [PATCH 05/18] Store 'renderers' as GArray in RedsState

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 2016-02-01 at 08:47 -0500, Frediano Ziglio wrote:
> > 
> > From: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> > 
> > Acked-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > ---
> >  server/display-channel.c | 13 +++++++------
> >  server/display-channel.h |  3 +--
> >  server/reds-private.h    |  1 +
> >  server/reds.c            | 20 ++++++++++++--------
> >  server/reds.h            |  5 ++---
> >  5 files changed, 23 insertions(+), 19 deletions(-)
> > 
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index f0d133a..2282847 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -1911,10 +1911,11 @@ void display_channel_create_surface(DisplayChannel
> > *display, uint32_t surface_id
> >  
> >      if (display->renderer == RED_RENDERER_INVALID) {
> >          int i;
> > -        for (i = 0; i < display->num_renderers; i++) {
> > -            surface->context.canvas = create_canvas_for_surface(display,
> > surface, display->renderers[i]);
> > +        for (i = 0; i < display->renderers->len; i++) {
> > +            uint32_t renderer = g_array_index(display->renderers, uint32_t,
> > i);
> > +            surface->context.canvas = create_canvas_for_surface(display,
> > surface, renderer);
> >              if (surface->context.canvas) {
> > -                display->renderer = display->renderers[i];
> > +                display->renderer = renderer;
> >                  break;
> >              }
> >          }
> > @@ -2030,8 +2031,9 @@ DisplayChannel* display_channel_new(RedWorker *worker,
> > int migrate, int stream_v
> >      static SpiceImageSurfacesOps image_surfaces_ops = {
> >          image_surfaces_get,
> >      };
> > +    GArray *renderers = reds_get_renderers(reds);
> >  
> > -    spice_return_val_if_fail(num_renderers > 0, NULL);
> > +    spice_return_val_if_fail(renderers->len > 0, NULL);
> >  
> >      spice_info("create display channel");
> >      display = (DisplayChannel *)red_worker_new_channel(
> > @@ -2063,8 +2065,7 @@ DisplayChannel* display_channel_new(RedWorker *worker,
> > int migrate, int stream_v
> >      stat_compress_init(&display->lz4_stat, "lz4", stat_clock);
> >  
> >      display->n_surfaces = n_surfaces;
> > -    display->num_renderers = num_renderers;
> > -    memcpy(display->renderers, renderers, sizeof(display->renderers));
> > +    display->renderers = g_array_ref(renderers);
> >      display->renderer = RED_RENDERER_INVALID;
> >  
> >      ring_init(&display->current_list);
> 
> Actually this is changing the way code behave and in theory introduce a race
> condition.
> In the previous code array was copied to channel, in the new code array is
> referenced
> in the channel. Potentially you could insert new rendered in the array after
> array is
> referenced and before is used. 

True, it is potentially a slight change in behavior. On the other hand, does it
make any sense to add new renderers after the server is already running? I am
almost tempted to add something like this to make it clear that this isn't
really supported.

diff --git a/server/reds.c b/server/reds.c
index 35442b0..f9251b9 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3779,6 +3779,9 @@ SPICE_GNUC_VISIBLE int
spice_server_is_server_mouse(SpiceServer *s)
 SPICE_GNUC_VISIBLE int spice_server_add_renderer(SpiceServer *s, const char
*name)
 {
     spice_assert(reds == s);
+    /* warn if we try to add new renderers after the spice server is already
+     * initialized */
+    if (s->main_channel != NULL)
+        g_warning("Please add renderers before initializing the spice server");
+
     if (!red_add_renderer(name)) {
         return -1;
     }

or even using g_return_if_fail() instead of just issuing a warning. Looking at
code that uses spice server, neither qemu nor xspice actually call
spice_server_add_renderer(). So it doesn't appear that any changes would be
required.


> In practice this should not happen. Actually in practice
> there is no reason why first channel should use a renderer and others other
> renderers.
> Considering that you cannot remove renderers from the list, that first working
> renderer
> are chosen and first channel succeeded this mean you will always use same
> renderer for
> all channels on same server. At this point rendered could be cached first time
> is tested
> (perhaps instead of copying the array we should just finding the renderer
> working).

Not sure I understand what you're suggesting here.

> Also in our environment seems a bit overkilling these dynamic allocated
> arrays.

Since this is a function that is only executed a couple of times during the
lifetime of the spice server, I don't see any reason to avoid it. And I
personally think it makes the code simpler to use these slightly higher-level
data types.

> Actually RED_RENDERER_LAST is two as 0 is used for invalid and 1 for the only
> real
> renderer available. I think event uint32_t is overkilling, uint8_t is enough.
> 
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index bf29cd3..34fb57f 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -168,8 +168,7 @@ struct DisplayChannel {
> >  
> >      MonitorsConfig *monitors_config;
> >  
> > -    uint32_t num_renderers;
> > -    uint32_t renderers[RED_RENDERER_LAST];
> > +    GArray *renderers;
> >      uint32_t renderer;
> >      int enable_jpeg;
> >      int enable_zlib_glz_wrap;
> > diff --git a/server/reds-private.h b/server/reds-private.h
> > index 285f226..60c38aa 100644
> > --- a/server/reds-private.h
> > +++ b/server/reds-private.h
> > @@ -196,6 +196,7 @@ struct RedsState {
> >      int default_channel_security;
> >      ChannelSecurityOptions *channels_security;
> >      const char *default_renderer;
> > +    GArray *renderers;
> >  
> >      int spice_port;
> >      int spice_secure_port;
> > diff --git a/server/reds.c b/server/reds.c
> > index feae0cc..0c94d6e 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -3440,6 +3440,7 @@ SPICE_GNUC_VISIBLE SpiceServer *spice_server_new(void)
> >      reds->default_channel_security =
> >          SPICE_CHANNEL_SECURITY_NONE | SPICE_CHANNEL_SECURITY_SSL;
> >      reds->default_renderer = "sw";
> > +    reds->renderers = g_array_sized_new(FALSE, TRUE, sizeof(uint32_t),
> > RED_RENDERER_LAST);
> >      reds->spice_port = -1;
> >      reds->spice_secure_port = -1;
> >      return reds;
> > @@ -3455,9 +3456,6 @@ static RendererInfo renderers_info[] = {
> >      {RED_RENDERER_INVALID, NULL},
> >  };
> >  
> > -uint32_t renderers[RED_RENDERER_LAST];
> > -uint32_t num_renderers = 0;
> > -
> >  static RendererInfo *find_renderer(const char *name)
> >  {
> >      RendererInfo *inf = renderers_info;
> > @@ -3470,14 +3468,14 @@ static RendererInfo *find_renderer(const char *name)
> >      return NULL;
> >  }
> >  
> > -static int red_add_renderer(const char *name)
> > +static int reds_add_renderer(RedsState *reds, const char *name)
> >  {
> >      RendererInfo *inf;
> >  
> > -    if (num_renderers == RED_RENDERER_LAST || !(inf = find_renderer(name)))
> > {
> > +    if (reds->renderers->len == RED_RENDERER_LAST || !(inf =
> > find_renderer(name))) {
> 
> Here the check was done to avoid overflow, can be removed or we could
> check that there are no repetitions on the array.
> 
> >          return FALSE;
> >      }
> > -    renderers[num_renderers++] = inf->id;
> > +    g_array_append_val(reds->renderers, inf->id);
> >      return TRUE;
> >  }
> >  
> > @@ -3488,7 +3486,7 @@ SPICE_GNUC_VISIBLE int spice_server_init(SpiceServer
> > *s, SpiceCoreInterface *cor
> >      spice_assert(reds == s);
> >      ret = do_spice_init(s, core);
> >      if (s->default_renderer) {
> > -        red_add_renderer(s->default_renderer);
> > +        reds_add_renderer(s, s->default_renderer);
> >      }
> >      return ret;
> >  }
> > @@ -3496,6 +3494,7 @@ SPICE_GNUC_VISIBLE int spice_server_init(SpiceServer
> > *s, SpiceCoreInterface *cor
> >  SPICE_GNUC_VISIBLE void spice_server_destroy(SpiceServer *s)
> >  {
> >      spice_assert(reds == s);
> > +    g_array_unref(s->renderers);
> >      reds_exit();
> >  }
> >  
> > @@ -3779,7 +3778,7 @@ SPICE_GNUC_VISIBLE int
> > spice_server_is_server_mouse(SpiceServer *s)
> >  SPICE_GNUC_VISIBLE int spice_server_add_renderer(SpiceServer *s, const char
> >  *name)
> >  {
> >      spice_assert(reds == s);
> > -    if (!red_add_renderer(name)) {
> > +    if (!reds_add_renderer(s, name)) {
> >          return -1;
> >      }
> >      s->default_renderer = NULL;
> > @@ -4027,3 +4026,8 @@ SPICE_GNUC_VISIBLE void
> > spice_server_set_keepalive_timeout(SpiceServer *s, int t
> >      reds->keepalive_timeout = timeout;
> >      spice_debug("keepalive timeout=%d", timeout);
> >  }
> > +
> > +GArray* reds_get_renderers(RedsState *reds)
> > +{
> > +    return reds->renderers;
> > +}
> 
> Why not incrementing reference here?
> 
> > diff --git a/server/reds.h b/server/reds.h
> > index e398607..4034199 100644
> > --- a/server/reds.h
> > +++ b/server/reds.h
> > @@ -63,6 +63,8 @@ int reds_get_agent_mouse(void); // used by inputs_channel
> >  int reds_has_vdagent(void); // used by inputs channel
> >  void reds_handle_agent_mouse_event(RedsState *reds, const VDAgentMouseState
> >  *mouse_state); // used by inputs_channel
> >  
> > +GArray* reds_get_renderers(RedsState *reds);
> > +
> >  enum {
> >      RED_RENDERER_INVALID,
> >      RED_RENDERER_SW,
> > @@ -70,9 +72,6 @@ enum {
> >      RED_RENDERER_LAST
> >  };
> >  
> > -extern uint32_t renderers[RED_RENDERER_LAST];
> > -extern uint32_t num_renderers;
> > -
> >  extern struct SpiceCoreInterfaceInternal *core;
> >  extern uint32_t streaming_video;
> >  extern SpiceImageCompression image_compression;
> > --
> > 2.4.3
> > 
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]