[Spice-devel] [PATCH 11/15] Store 'renderers' as GArray in RedsState
Frediano Ziglio
fziglio at redhat.com
Fri Jan 22 07:27:04 PST 2016
>
> From: Jonathon Jongsma <jjongsma at redhat.com>
>
> ---
> server/display-channel.c | 13 +++++++------
> server/display-channel.h | 3 +--
> server/reds-private.h | 2 ++
> server/reds.c | 20 ++++++++++++--------
> server/reds.h | 5 ++---
> 5 files changed, 24 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);
> 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 75a4f59..7a3b524 100644
> --- a/server/reds-private.h
> +++ b/server/reds-private.h
> @@ -196,6 +196,8 @@ struct RedsState {
> int default_channel_security;
> ChannelSecurityOptions *channels_security;
> const char *default_renderer;
> + GArray *renderers;
> +
> };
>
> #endif
> diff --git a/server/reds.c b/server/reds.c
> index 0a49f3d..6f3d9a7 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3446,6 +3446,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);
> return reds;
> }
>
> @@ -3459,9 +3460,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;
> @@ -3474,14 +3472,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))) {
> return FALSE;
> }
> - renderers[num_renderers++] = inf->id;
> + g_array_append_val(reds->renderers, inf->id);
> return TRUE;
> }
>
> @@ -3492,7 +3490,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;
> }
> @@ -3500,6 +3498,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();
> }
>
> @@ -3783,7 +3782,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;
> @@ -4031,3 +4030,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;
> +}
> 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;
Acked-by: Frediano Ziglio <fziglio at redhat.com>
One thing I don't like of Glib (some part of it) is that make C less type safe than it is.
Try to compile and run this program
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <assert.h>
#include <glib.h>
int main(void)
{
GArray* a = g_array_sized_new(FALSE, TRUE, sizeof(uint8_t), 1024);
uint32_t n = g_array_index(a, uint32_t, 1000);
g_array_unref(a);
assert(n == 0);
return 0;
}
enable all warnings... any warning/error at all? Nothing by the compiler.
Obviously in this small program. But look at the patch code. The allocation line
can be really far from the usage. And if you update one and not the other.. well..
sometimes it works sometimes it have strange behavior, sometimes it crashes.
All without any compiler concerns. This IMHO introduces a code maintenance in the
long run.
Frediano
More information about the Spice-devel
mailing list