[Spice-devel] [PATCH spice-server] stat-file: Avoid compiler warning

[Uri Lublin]

On 02/02/2017 02:36 PM, Frediano Ziglio wrote:
>>
>> On 02/02/2017 11:52 AM, Frediano Ziglio wrote:
>>> Some gcc version reports this error:
>>>
>>> stat-file.c: In function 'stat_file_add_node':
>>> stat-file.c:180:15: error: 'node' may be used uninitialized in this
>>> function
>>> [-Werror=maybe-uninitialized]
>>>      g_strlcpy(node->name, name, sizeof(node->name));
>>>                ^~~~
>>> cc1: all warnings being treated as errors
>>>
>>> This warning is a false positive as this loop:
>>>     for (ref = 0; ref <= stat_file->max_nodes; ref++) {
>>>         node = &stat_file->stat->nodes[ref];
>>>         ...
>>>     }
>>> will always iterate at least once.
>>>
>>> This patch rewrite the loop in order to make more compilers
>>> understand that the NULL check is useless.
>>>
>>> Reported-by: Christophe Fergeau <cfergeau at redhat.com>
>>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>>> ---
>>>  server/stat-file.c | 26 ++++++++++++--------------
>>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/server/stat-file.c b/server/stat-file.c
>>> index c23f4f5..05ad0ef 100644
>>> --- a/server/stat-file.c
>>> +++ b/server/stat-file.c
>>> @@ -162,25 +162,23 @@ stat_file_add_node(RedStatFile *stat_file,
>>> StatNodeRef parent, const char *name,
>>>              return ref;
>>>          }
>>>      }
>>> -    if (stat_file->stat->num_of_nodes >= stat_file->max_nodes ||
>>> stat_file->stat == NULL) {
>>> -        pthread_mutex_unlock(&stat_file->lock);
>>> -        return INVALID_STAT_REF;
>>> -    }
>>
>> Hi Frediano,
>>
>> Why did you remove this check ?
>> I think it is important.
>>
>> Uri.
>>
>
> It's implicit in the loop.
> If num_of_nodes >= max_nodes means that there are no free nodes
> so all nodes should haveSPICE_STAT_NODE_FLAG_ENABLED set,
> loop will exit and function will return INVALID_STAT_REF.
> However I just realized that the test ref <= stat_file->max_nodes
> it's a off-by-one, should be ref < stat_file->max_nodes !!
>
> Frediano

Right, the off-by-one in the loop is protected by this condition.
This is why it's important.
But I prefer to change the loop condition, as you suggest.

What about stat_file->stat,
Is it guaranteed to not be NULL ?

Uri.

>
>>> -    stat_file->stat->generation++;
>>> -    stat_file->stat->num_of_nodes++;
>>>      for (ref = 0; ref <= stat_file->max_nodes; ref++) {
>>>          node = &stat_file->stat->nodes[ref];
>>> -        if (!(node->flags & SPICE_STAT_NODE_FLAG_ENABLED)) {
>>> -            break;
>>> +        if (!!(node->flags & SPICE_STAT_NODE_FLAG_ENABLED)) {
>>> +            continue;
>>>          }
>>> +        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);
>>> +        g_strlcpy(node->name, name, sizeof(node->name));
>>> +        reds_insert_stat_node(stat_file, parent, ref);
>>> +        pthread_mutex_unlock(&stat_file->lock);
>>> +        return ref;
>>>      }
>>> -    spice_assert(!(node->flags & SPICE_STAT_NODE_FLAG_ENABLED));
>>> -    node->value = 0;
>>> -    node->flags = SPICE_STAT_NODE_FLAG_ENABLED | (visible ?
>>> SPICE_STAT_NODE_FLAG_VISIBLE : 0);
>>> -    g_strlcpy(node->name, name, sizeof(node->name));
>>> -    reds_insert_stat_node(stat_file, parent, ref);
>>>      pthread_mutex_unlock(&stat_file->lock);
>>> -    return ref;
>>> +    return INVALID_STAT_REF;
>>>  }
>>>
>>>  uint64_t *
>>>
>>
>>