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

[Uri Lublin]

On 12/10/2017 04:35 PM, Frediano Ziglio wrote:
>>>
>>> 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.

You are right. I made a mistake.
The problem reported is in stat_file_add_counter()


We can fix it by locking/unlocking there:
diff --git a/server/stat-file.c b/server/stat-file.c
index 2797fd739..84f409a46 100644
--- a/server/stat-file.c
+++ b/server/stat-file.c
@@ -189,8 +189,10 @@ stat_file_add_counter(RedStatFile *stat_file, 
StatNodeRef parent, const char *na
      if (ref == INVALID_STAT_REF) {
          return NULL;
      }
+    pthread_mutex_lock(&stat_file->lock);
      node = &stat_file->stat->nodes[ref];
      node->flags |= SPICE_STAT_NODE_FLAG_VALUE;
+    pthread_mutex_unlock(&stat_file->lock);
      return &node->value;
  }


Your solution below also looks good to me
and does not add another lock, but probably
better to pass "additional_flags" (or "extra_flags")
instead of "flags" such that visible ? ... would appear
once as it is currently in the code.

Thanks,
     Uri.

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