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

[Pavel Grunt]

On Tue, 2017-03-07 at 03:18 -0500, Frediano Ziglio wrote:
> > 
> > On Mon, 2017-03-06 at 18:22 +0000, Frediano Ziglio wrote:
> > > The default memset(0) on the node does not init it correctly.
> > 
> > yeah, nice catch, INVALID_STAT_REF = ~0
> > 
> > > Do so to avoid other nodes/counters appended to unwanted nodes.
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  server/red-channel.c | 2 ++
> > >  server/stat.h        | 8 ++++++++
> > >  2 files changed, 10 insertions(+)
> > > 
> > > diff --git a/server/red-channel.c b/server/red-channel.c
> > > index 8ae6ece..8206f4c 100644
> > > --- a/server/red-channel.c
> > > +++ b/server/red-channel.c
> > > @@ -300,6 +300,8 @@ 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;
> > > +
> > > +    stat_init_node_invalid(&self->priv->stat);
> > >  }
> > >  
> > >  
> > > diff --git a/server/stat.h b/server/stat.h
> > > index 5255efa..980fb07 100644
> > > --- a/server/stat.h
> > > +++ b/server/stat.h
> > > @@ -70,6 +70,14 @@ stat_remove_counter(SpiceServer *reds,
> > > RedStatCounter *counter)
> > >  #endif /* RED_STATISTICS */
> > >  
> > >  static inline void
> > > +stat_init_node_invalid(RedStatNode *node)
> > 
> > maybe 'invalidate' instead of 'invalid', or _set_invalid
> > 
> > Also the argument can be marked as unused
> 
> Yes,
> 
> > > +{
> > > +#ifdef RED_STATISTICS
> > > +    node->ref = INVALID_STAT_REF;
> > > +#endif
> > > +}
> > > +
> > > +static inline void
> > >  stat_inc_counter(RedStatCounter counter, uint64_t value)
> > >  {
> > >  #ifdef RED_STATISTICS
> > 
> > What about using / improving stat_init_node or stat_remove_node
> > 
> > Wouldn't be easier to change the INVALID_STAT_REF define ?
> > 
> > 
> > Thanks,
> > Pavel
> > 
> > 
> 
> INVALID_STAT_REF is currently part of the ABI for the file.
> Unless we use a different invalid for stat-file API
That's what I meant


>  one way
> is this (less hacky than I though):
> 
> 
> diff --git a/server/reds.c b/server/reds.c
> index fc720a3..e910404 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -355,22 +355,23 @@ static void reds_link_free(RedLinkInfo *link)
>  void stat_init_node(RedStatNode *node, SpiceServer *reds, const
> RedStatNode *parent,
>                      const char *name, int visible)
>  {
> -    StatNodeRef parent_ref = parent ? parent->ref :
> INVALID_STAT_REF;
> -    node->ref = stat_file_add_node(reds->stat_file, parent_ref,
> name, visible);
> +    StatNodeRef parent_ref = (parent ? parent->masked_ref : 0) ^
> INVALID_STAT_REF;
> +    node->masked_ref =
> +        stat_file_add_node(reds->stat_file, parent_ref, name,
> visible) ^ INVALID_STAT_REF;
>  }
>  
>  void stat_remove_node(SpiceServer *reds, RedStatNode *node)
>  {
> -    if (node->ref != INVALID_STAT_REF) {
> -        stat_file_remove_node(reds->stat_file, node->ref);
> -        node->ref = INVALID_STAT_REF;
> +    if (node->masked_ref) {
> +        stat_file_remove_node(reds->stat_file, node->masked_ref ^
> INVALID_STAT_REF);
> +        node->masked_ref = 0;
>      }
>  }
>  
>  void stat_init_counter(RedStatCounter *counter, SpiceServer *reds,
>                         const RedStatNode *parent, const char *name,
> int visible)
>  {
> -    StatNodeRef parent_ref = parent ? parent->ref :
> INVALID_STAT_REF;
> +    StatNodeRef parent_ref = (parent ? parent->masked_ref : 0) ^
> INVALID_STAT_REF;
>      counter->counter =
>          stat_file_add_counter(reds->stat_file, parent_ref, name,
> visible);
>  }
> diff --git a/server/stat.h b/server/stat.h
> index 5255efa..d191063 100644
> --- a/server/stat.h
> +++ b/server/stat.h
> @@ -32,7 +32,10 @@ typedef struct {
>  
>  typedef struct {
>  #ifdef RED_STATISTICS
> -    uint32_t ref;
> +    /* to allow the use of 0 as default invalid value (ie memset(0)
> +     * to work) this value is "masked" with INVALID_STAT_REF
> +     * (which is part of file ABI so cannot be changes) */
> +    uint32_t masked_ref;
>  #endif
>  } RedStatNode;
>  
> 
> 
> Does it make sense?

The original patch is better / easier to understand. Can you add a
comment to red_channel_init(), why we need to invalidate the node.

Thanks,
Pavel

> 
> Frediano