[Spice-devel] [PATCH spice-server v2 2/2] red-channel: Initialize base statistic node creating the object
Christophe Fergeau
cfergeau at redhat.com
Fri Mar 24 15:48:42 UTC 2017
From a somehow quick look, it seems only DisplayChannel and the VMC
channel are using RedChannel::stat. The DisplayChannel stat node needs
to have the RedWorker stat node as a parent, the VMC channel does not
need a parent node.
The cursor channel has some code to init its stat node, but it's then
unused?
I really don't like the approach in this patch, far too invasive for
something which is optional.
The stat counters only seem to be used once we have a
DisplayChannelClient though, so we could init everything after the
DisplayChannel has been created, ie:
worker->display_channel = display_channel_new(reds, qxl, &worker->core, FALSE,
reds_get_streaming_video(reds),
reds_get_video_codecs(reds),
init_info.n_surfaces);
channel = RED_CHANNEL(worker->display_channel);
display_channel_init_stats(display_channel, &worker->stat, "display_channel");
And I'd kill RedChannel::stat, and only have it in the 2 channels which really
needs it. Or this is one of the cases where we could probably have some kind of
slightly higher level path-based API rather than needing a pointer to the
relevant StatNode (eg red_stat_new_counter("/worker/display_channel/cache_hits", ...);
RedStatCounter *counter = red_stat_get_counter("/worker/display_channel/cache_hits");)
Christophe
On Wed, Mar 08, 2017 at 02:55:31PM +0000, Frediano Ziglio wrote:
> Avoid usage of not initialized statistic node.
> This happened for DisplayChannel which was initializing
> statistics in the object constructor.
>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> server/cursor-channel.c | 4 +++-
> server/cursor-channel.h | 5 +++--
> server/display-channel.c | 4 +++-
> server/display-channel.h | 3 ++-
> server/red-channel.c | 24 +++++++++++++++---------
> server/red-channel.h | 2 --
> server/red-worker.c | 11 ++++++-----
> server/spicevmc.c | 4 +++-
> 8 files changed, 35 insertions(+), 22 deletions(-)
>
> Changes since v1:
> - rebased
>
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 9d21962..48feab7 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -292,7 +292,8 @@ static void cursor_channel_send_item(RedChannelClient *rcc, RedPipeItem *pipe_it
> }
>
> CursorChannel* cursor_channel_new(RedsState *server, QXLInstance *qxl,
> - const SpiceCoreInterfaceInternal *core)
> + const SpiceCoreInterfaceInternal *core,
> + const RedStatNode *stat_node)
> {
> spice_debug("create cursor channel");
> return g_object_new(TYPE_CURSOR_CHANNEL,
> @@ -303,6 +304,7 @@ CursorChannel* cursor_channel_new(RedsState *server, QXLInstance *qxl,
> "migration-flags", 0,
> "qxl", qxl,
> "handle-acks", TRUE,
> + "stat-node", stat_node,
> NULL);
> }
>
> diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> index 6bf8486..a7f862e 100644
> --- a/server/cursor-channel.h
> +++ b/server/cursor-channel.h
> @@ -55,8 +55,9 @@ GType cursor_channel_get_type(void) G_GNUC_CONST;
> * provided as helper functions and should only be called from the
> * CursorChannel thread.
> */
> -CursorChannel* cursor_channel_new (RedsState *server, QXLInstance *qxl,
> - const SpiceCoreInterfaceInternal *core);
> +CursorChannel* cursor_channel_new(RedsState *server, QXLInstance *qxl,
> + const SpiceCoreInterfaceInternal *core,
> + const RedStatNode *stat_node);
>
> /**
> * Cause the channel to disconnect all clients
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 67a77ef..9d1bbf9 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1987,7 +1987,8 @@ DisplayChannel* display_channel_new(RedsState *reds,
> const SpiceCoreInterfaceInternal *core,
> int migrate, int stream_video,
> GArray *video_codecs,
> - uint32_t n_surfaces)
> + uint32_t n_surfaces,
> + const RedStatNode *stat_node)
> {
> DisplayChannel *display;
>
> @@ -2004,6 +2005,7 @@ DisplayChannel* display_channel_new(RedsState *reds,
> "n-surfaces", n_surfaces,
> "video-codecs", video_codecs,
> "handle-acks", TRUE,
> + "stat-node", stat_node,
> NULL);
> if (display) {
> display_channel_set_stream_video(display, stream_video);
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 3d4accf..571685e 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -204,7 +204,8 @@ DisplayChannel* display_channel_new (RedsState
> int migrate,
> int stream_video,
> GArray *video_codecs,
> - uint32_t n_surfaces);
> + uint32_t n_surfaces,
> + const RedStatNode *stat_node);
> void display_channel_create_surface (DisplayChannel *display, uint32_t surface_id,
> uint32_t width, uint32_t height,
> int32_t stride, uint32_t format, void *line_0,
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 8e4d582..e0fb7f1 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -108,7 +108,8 @@ enum {
> PROP_TYPE,
> PROP_ID,
> PROP_HANDLE_ACKS,
> - PROP_MIGRATION_FLAGS
> + PROP_MIGRATION_FLAGS,
> + PROP_STAT_NODE,
> };
>
> static void
> @@ -172,6 +173,11 @@ red_channel_set_property(GObject *object,
> case PROP_MIGRATION_FLAGS:
> self->priv->migration_flags = g_value_get_uint(value);
> break;
> + case PROP_STAT_NODE:
> + if (g_value_get_pointer(value)) {
> + self->priv->stat = *((const RedStatNode *)g_value_get_pointer(value));
> + }
> + break;
> default:
> G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
> }
> @@ -287,6 +293,14 @@ red_channel_class_init(RedChannelClass *klass)
> G_PARAM_CONSTRUCT_ONLY |
> G_PARAM_STATIC_STRINGS);
> g_object_class_install_property(object_class, PROP_MIGRATION_FLAGS, spec);
> +
> + spec = g_param_spec_pointer("stat-node",
> + "stat-node",
> + "The statistics node associated with this channel",
> + G_PARAM_WRITABLE |
> + G_PARAM_CONSTRUCT_ONLY |
> + G_PARAM_STATIC_STRINGS);
> + g_object_class_install_property(object_class, PROP_STAT_NODE, spec);
> }
>
> static void
> @@ -361,14 +375,6 @@ int red_channel_is_waiting_for_migrate_data(RedChannel *channel)
> return red_channel_client_is_waiting_for_migrate_data(rcc);
> }
>
> -void red_channel_init_stat_node(RedChannel *channel, const RedStatNode *parent, const char *name)
> -{
> - spice_return_if_fail(channel != NULL);
> -
> - // TODO check not already initialized
> - stat_init_node(&channel->priv->stat, channel->priv->reds, parent, name, TRUE);
> -}
> -
> const RedStatNode *red_channel_get_stat_node(RedChannel *channel)
> {
> return &channel->priv->stat;
> diff --git a/server/red-channel.h b/server/red-channel.h
> index 44282f6..ba1a584 100644
> --- a/server/red-channel.h
> +++ b/server/red-channel.h
> @@ -134,8 +134,6 @@ GType red_channel_get_type(void) G_GNUC_CONST;
> void red_channel_add_client(RedChannel *channel, RedChannelClient *rcc);
> void red_channel_remove_client(RedChannel *channel, RedChannelClient *rcc);
>
> -void red_channel_init_stat_node(RedChannel *channel, const RedStatNode *parent, const char *name);
> -
> void red_channel_register_client_cbs(RedChannel *channel, const ClientCbs *client_cbs, gpointer cbs_data);
> // caps are freed when the channel is destroyed
> void red_channel_set_common_cap(RedChannel *channel, uint32_t cap);
> diff --git a/server/red-worker.c b/server/red-worker.c
> index c88034b..2f7a6de 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -1310,6 +1310,7 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> Dispatcher *dispatcher;
> RedsState *reds = red_qxl_get_server(qxl->st);
> RedChannel *channel;
> + RedStatNode stat_node;
>
> red_qxl_get_init_info(qxl, &init_info);
>
> @@ -1356,21 +1357,21 @@ RedWorker* red_worker_new(QXLInstance *qxl,
>
> worker->event_timeout = INF_EVENT_WAIT;
>
> - worker->cursor_channel = cursor_channel_new(reds, qxl,
> - &worker->core);
> + stat_init_node(&stat_node, reds, &worker->stat, "cursor_channel", TRUE);
> + worker->cursor_channel = cursor_channel_new(reds, qxl, &worker->core, &stat_node);
> channel = RED_CHANNEL(worker->cursor_channel);
> - red_channel_init_stat_node(channel, &worker->stat, "cursor_channel");
> red_channel_register_client_cbs(channel, client_cursor_cbs, dispatcher);
> g_object_set_data(G_OBJECT(channel), "dispatcher", dispatcher);
> reds_register_channel(reds, channel);
>
> // TODO: handle seemless migration. Temp, setting migrate to FALSE
> + stat_init_node(&stat_node, reds, &worker->stat, "display_channel", TRUE);
> worker->display_channel = display_channel_new(reds, qxl, &worker->core, FALSE,
> reds_get_streaming_video(reds),
> reds_get_video_codecs(reds),
> - init_info.n_surfaces);
> + init_info.n_surfaces,
> + &stat_node);
> channel = RED_CHANNEL(worker->display_channel);
> - red_channel_init_stat_node(channel, &worker->stat, "display_channel");
> red_channel_register_client_cbs(channel, client_display_cbs, dispatcher);
> g_object_set_data(G_OBJECT(channel), "dispatcher", dispatcher);
> reds_register_channel(reds, channel);
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index f61ffc9..60f26ae 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -233,7 +233,6 @@ red_vmc_channel_constructed(GObject *object)
> client_cbs.connect = spicevmc_connect;
> red_channel_register_client_cbs(RED_CHANNEL(self), &client_cbs, NULL);
>
> - red_channel_init_stat_node(RED_CHANNEL(self), NULL, "spicevmc");
> const RedStatNode *stat = red_channel_get_stat_node(RED_CHANNEL(self));
> stat_init_counter(&self->in_data, reds, stat, "in_data", TRUE);
> stat_init_counter(&self->in_compressed, reds, stat, "in_compressed", TRUE);
> @@ -273,6 +272,7 @@ static RedVmcChannel *red_vmc_channel_new(RedsState *reds, uint8_t channel_type)
> {
> GType gtype = G_TYPE_NONE;
> static uint8_t id[SPICE_END_CHANNEL] = { 0, };
> + RedStatNode stat_node;
>
> switch (channel_type) {
> case SPICE_CHANNEL_USBREDIR:
> @@ -288,6 +288,7 @@ static RedVmcChannel *red_vmc_channel_new(RedsState *reds, uint8_t channel_type)
> g_error("Unsupported channel_type for red_vmc_channel_new(): %u", channel_type);
> return NULL;
> }
> + stat_init_node(&stat_node, reds, NULL, "spicevmc", TRUE);
> return g_object_new(gtype,
> "spice-server", reds,
> "core-interface", reds_get_core_interface(reds),
> @@ -296,6 +297,7 @@ static RedVmcChannel *red_vmc_channel_new(RedsState *reds, uint8_t channel_type)
> "handle-acks", FALSE,
> "migration-flags",
> (SPICE_MIGRATE_NEED_FLUSH | SPICE_MIGRATE_NEED_DATA_TRANSFER),
> + "stat-node", &stat_node,
> NULL);
> }
>
> --
> 2.9.3
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170324/f2d502f5/attachment-0001.sig>
More information about the Spice-devel
mailing list