>
> 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.
All channels use RedChannel::stat.
> 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.
>
I think so too however at the end I ended up with this.
The problem is that many of the counters are initialized in constructor
so before any method can be called. Changing the way statistics are actually
generated is even more invasive that this approach.
> 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");
>
This is the way is done currently and as I said this does not work
as the counters are initialized in display_channel_new so before
display_channel_init_stats is called.
> 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");)
>
Currently for display/cursor channels the path depends on the worker too.
Also some counters on the same path are created in RedChannelClient.
> Christophe
>
Frediano
>
>
>
> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel