[Spice-devel] [PATCH 05/18] Store 'renderers' as GArray in RedsState
Jonathon Jongsma
jjongsma at redhat.com
Tue Feb 2 17:52:03 CET 2016
On Mon, 2016-02-01 at 08:47 -0500, Frediano Ziglio wrote:
> >
> > From: Jonathon Jongsma <jjongsma at redhat.com>
> >
> > Acked-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> > 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list