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

[Frediano Ziglio]

> 
> 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 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?

Frediano