[Spice-devel] [PATCH 05/18] Store 'renderers' as GArray in RedsState
Frediano Ziglio
fziglio at redhat.com
Wed Feb 10 12:17:06 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.
>
Yes, this was one of my point. We copy the array to use later but actually we
suppose the array never changes so what's the point of having a copy.
We are touching the code so why don't change the approach.
Instead of accessing the array directly ask the server to provide a rendered.
So on first call the reds.c code will detect the renderer and give it back to caller
while caching the result that will be used on later calls.
Does it make sense?
> 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.
>
My point was (well one of the point) that copying the array was useless.
> > 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;
> > > +}
> >
So I would replace this with a
uint32_t reds_get_renderer(RedsState *reds, whatever needed to get it)
{
if (reds->renderer == INVALID) {
reds->renderer = compute_renderer(params);
}
return reds->renderer;
}
> > 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