[Spice-devel] [PATCH spice-server v3] red-channel: Initialize statistic node correctly

[Frediano Ziglio]

> 
> On Thu, Mar 30, 2017 at 01:45:05PM +0100, Frediano Ziglio wrote:
> > The default memset(0) on the node did not init it correctly.
> 
> "Doing a memset(0) on a SpiceStatNode does not create an empty/unattached
> node, but instead creates a node whose parent is the node '0'. This node
> could be non-existing, or totally unrelated to the empty node we wanted
> to create."
> 

SpiceStatNode contains a reference to the node which is basically
an index (0-based) on an array inside the statistic file.
The value ~0 is special and for a parent/node means invalid and the
child will appended as sibling from the root.
The memset(0) basically will initialize the node to the first node
in the array whatever is it. So node/counters created with parent as
this not initialized node will be child of the node 0
(being the first will be the first node/counter created).

> > This patch create the 0 node as soon as the file is created
> > to use as default memset(0) compatible node.
> 
> "This patch creates a default '0' node as soon as the file is created.
> This will make the behaviour of 0-filled nodes more in line with what
> one would expect."
> 

Yes. As all counters are created for channels creating a "default_channel"
node make sense and being 0 make the trick work.

> I think the log becomes a lot clearer as to what you are fixing and what
> the problem was with the suggested amendments.
> 
> Looks good to me, though I'm not overly familiar with the stat code.
> 
> Christophe
> 

Frediano

> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/reds.c                 |  4 ++++
> >  server/tests/test-stat-file.c | 19 +++++++++++++++++++
> >  2 files changed, 23 insertions(+)
> > 
> > Changes since v2:
> > - this patch create an additional node making 0 a valid node.
> >   This also make clear that the counters under it apply to a channel.
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index 0b6ca12..839ffe7 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -3500,6 +3500,10 @@ SPICE_GNUC_VISIBLE SpiceServer
> > *spice_server_new(void)
> >      reds->config->exit_on_disconnect = FALSE;
> >  #ifdef RED_STATISTICS
> >      reds->stat_file = stat_file_new(REDS_MAX_STAT_NODES);
> > +    /* Create an initial node. This will be the 0 node making easier
> > +     * to initialize node references.
> > +     */
> > +    stat_file_add_node(reds->stat_file, INVALID_STAT_REF,
> > "default_channel", TRUE);
> >  #endif
> >      reds->listen_socket = -1;
> >      reds->secure_listen_socket = -1;
> > diff --git a/server/tests/test-stat-file.c b/server/tests/test-stat-file.c
> > index 63a2a2b..901985a 100644
> > --- a/server/tests/test-stat-file.c
> > +++ b/server/tests/test-stat-file.c
> > @@ -89,11 +89,30 @@ static void stat_file(void)
> >      stat_file_free(stat_file);
> >  }
> >  
> > +/* make sure first node is node 0 */
> > +static void stat_file_start(void)
> > +{
> > +    RedStatFile *stat_file;
> > +    StatNodeRef ref;
> > +
> > +    /* create */
> > +    stat_file = stat_file_new(10);
> > +    g_assert_nonnull(stat_file);
> > +
> > +    ref = stat_file_add_node(stat_file, INVALID_STAT_REF, "ZERO", TRUE);
> > +    g_assert_cmpuint(ref,==,0);
> > +
> > +    stat_file_unlink(stat_file);
> > +    stat_file_free(stat_file);
> > +}
> > +
> > +
> >  int main(int argc, char *argv[])
> >  {
> >      g_test_init(&argc, &argv, NULL);
> >  
> >      g_test_add_func("/server/stat-file", stat_file);
> > +    g_test_add_func("/server/stat-file-start", stat_file_start);
> >  
> >      return g_test_run();
> >  }