[Spice-devel] [PATCH spice-server v2 1/2] red-channel: Initialize statistic node correctly

[Frediano Ziglio]

> 
> On Wed, Mar 08, 2017 at 02:55:30PM +0000, Frediano Ziglio wrote:
> > The default memset(0) on the node does not init it correctly.
> > Do so to avoid other nodes/counters appended to unwanted nodes.
> 
> I don't understand this sentence. Do you mean it's required to correctly
> mark the node as invalid, otherwise we'll append other nodes/counters to
> incorrect nodes?
> 

Potentially will append some nodes on node 0 (which is valid).
If you append to ~0 are "appended" at the end, a bit better.

> > This is due to INVALID_STAT_REF being ~0. We could not change
> > this value easily as this is used as ABI for the statistics file.
> 
> Fwiw I would not consider the ABI of the stat file as something we need
> to provide a strong guarantee on.
> 

I think there's a ABI field in the file... but retaining compatibility
could be not worth not maintain. Would be good if at least the reds_stat
utility could handle all.
I proposed also a patch to make 0-filled the invalid but was not
really readable too.

> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/red-channel.c | 4 ++++
> >  server/stat.h        | 8 ++++++++
> >  2 files changed, 12 insertions(+)
> > 
> > Changes since v1:
> > - improve comment;
> > - rename initialization function.
> > 
> > diff --git a/server/red-channel.c b/server/red-channel.c
> > index 8ae6ece..8e4d582 100644
> > --- a/server/red-channel.c
> > +++ b/server/red-channel.c
> > @@ -300,6 +300,10 @@ red_channel_init(RedChannel *self)
> >      self->priv->client_cbs.connect = red_channel_client_default_connect;
> >      self->priv->client_cbs.disconnect =
> >      red_channel_client_default_disconnect;
> >      self->priv->client_cbs.migrate = red_channel_client_default_migrate;
> > +
> > +    /* this is necessary as invalid value is not all fields 0
> > +     * so the default memset(0) is not enough */
> 
> /* This is necessary as a 0-filled RedStatNode is not an empty invalid
>  * node, some of its fields must be set to magic values first */
> 
> > +    stat_node_set_invalid(&self->priv->stat);
> 
> stat_reset_node() maybe?
> 
> Christophe
> 

Usually we use reset as a restore initial state from some valid state,
in this case we start from an uninitialized state.
I think was kind of stat_node_init_invalid or stat_node_init_empty.

Frediano