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

[Frediano Ziglio]

> 
> On Thu, 2017-02-02 at 12:46 +0000, 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 3fe3890..96c3bc1 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;
> > -    }
> > -    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];
> 
> I was going to mention that you removed the check for stat_file->state
> == NULL above and you're using it directly here. But looking at the
> code, I see that stat_file->stat was already dereferenced in this
> function even before this check, so it was already a pointless check.
> 

Send a new version with some more comments (no code changes)

> However, the fact that you removed the check for num_of_nodes >=
> max_nodes means that stat->num_of_nodes is now only incremented and
> decremented in this file but is never used except in tools/reds_stat.c.
> Is it worth keeping around?
> 

Yes, the field is important (not only for ABI).
The question is how is used and how should be used.
Currently it's the number of node used.
On the other way is used by reds_stat to compute the size of
the part of the file to map into memory. Now consider this
- file is created with maximum 100 elements
- 10 nodes/counters are added
- first 5 nodes/counters are removed
Now max_nodes == 100 and num_of_nodes == 5 however reds_stat
will try to map only 5 elements which are empty.
Currently everything works as there are no removal calls
but being num_of_nodes used for mapping should probably
contain the same value as max_nodes.

> Otherwise it looks fine.
> 
> Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
> 
> 
> 
> > -        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 *
> 

Frediano