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@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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel