[Spice-devel] [PATCH 05/18] Store 'renderers' as GArray in RedsState
Frediano Ziglio
fziglio at redhat.com
Mon Feb 1 14:47:38 CET 2016
>
> 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. 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).
Also in our environment seems a bit overkilling these dynamic allocated arrays.
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
More information about the Spice-devel
mailing list