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

[Jonathon Jongsma]

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.

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?

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 *