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

[Frediano Ziglio]

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

There are multiple usage of stat_file->stat before in the
same function so it's worthless to check at that point.
And stat_file->stat is set at initialization and never changed
so beside memory corruption so can't be NULL.

Frediano

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