[Spice-devel] [server PATCH 4/4] stat_file_add_node: add "locked" comment

[Frediano Ziglio]

> > 
> > Coverity complains node->flags is accessed without locking stat_file->lock.
> > However the function stat_file_add_node is only called (from
> > stat_file_add_counter) when the lock is taken.
> > 
> 
> No, this is false. The function handle the lock.
> The problem happens because stat_file_add_counter add a flag without locking.
> However in this case this is not much of a problem as the this is the last
> change of
> the flag and maximum cause a temporary view of the node as not counter.
> 
> > Add comment so it's clear.
> > 
> > Signed-off-by: Uri Lublin <uril at redhat.com>
> > ---
> >  server/stat-file.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/server/stat-file.c b/server/stat-file.c
> > index 2797fd739..9aff8cd72 100644
> > --- a/server/stat-file.c
> > +++ b/server/stat-file.c
> > @@ -139,6 +139,7 @@ static void reds_insert_stat_node(RedStatFile
> > *stat_file,
> > StatNodeRef parent, St
> >      }
> >  }
> >  
> > +/* Called with stat_file->lock locked */
> >  StatNodeRef
> >  stat_file_add_node(RedStatFile *stat_file, StatNodeRef parent, const char
> >  *name, int visible)
> >  {
> 

This should work


diff --git a/server/stat-file.c b/server/stat-file.c
index 2797fd73..c52c266c 100644
--- a/server/stat-file.c
+++ b/server/stat-file.c
@@ -139,8 +139,9 @@ static void reds_insert_stat_node(RedStatFile *stat_file, StatNodeRef parent, St
     }
 }
 
-StatNodeRef
-stat_file_add_node(RedStatFile *stat_file, StatNodeRef parent, const char *name, int visible)
+static StatNodeRef
+stat_file_add_node_flags(RedStatFile *stat_file, StatNodeRef parent, const char *name,
+                         uint32_t flags)
 {
     StatNodeRef ref;
     SpiceStatNode *node;
@@ -169,8 +170,7 @@ stat_file_add_node(RedStatFile *stat_file, StatNodeRef parent, const char *name,
         stat_file->stat->generation++;
         stat_file->stat->num_of_nodes++;
         node->value = 0;
-        node->flags = SPICE_STAT_NODE_FLAG_ENABLED |
-                      (visible ? SPICE_STAT_NODE_FLAG_VISIBLE : 0);
+        node->flags = SPICE_STAT_NODE_FLAG_ENABLED | flags;
         g_strlcpy(node->name, name, sizeof(node->name));
         reds_insert_stat_node(stat_file, parent, ref);
         pthread_mutex_unlock(&stat_file->lock);
@@ -180,17 +180,25 @@ stat_file_add_node(RedStatFile *stat_file, StatNodeRef parent, const char *name,
     return INVALID_STAT_REF;
 }
 
+StatNodeRef
+stat_file_add_node(RedStatFile *stat_file, StatNodeRef parent, const char *name, int visible)
+{
+    uint32_t flags = visible ? SPICE_STAT_NODE_FLAG_VISIBLE : 0;
+    return stat_file_add_node_flags(stat_file, parent, name, flags);
+}
+
 uint64_t *
 stat_file_add_counter(RedStatFile *stat_file, StatNodeRef parent, const char *name, int visible)
 {
-    StatNodeRef ref = stat_file_add_node(stat_file, parent, name, visible);
+    uint32_t flags = visible ? SPICE_STAT_NODE_FLAG_VISIBLE : 0;
+    flags |= SPICE_STAT_NODE_FLAG_VALUE;
+    StatNodeRef ref = stat_file_add_node_flags(stat_file, parent, name, flags);
     SpiceStatNode *node;
 
     if (ref == INVALID_STAT_REF) {
         return NULL;
     }
     node = &stat_file->stat->nodes[ref];
-    node->flags |= SPICE_STAT_NODE_FLAG_VALUE;
     return &node->value;
 }


 
Frediano