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

[Frediano Ziglio]

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

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