[Spice-devel] [PATCH spice-server v2 2/3] Fix removing a node

[Frediano Ziglio]

> 
> In the short log, I'd talk about "node removal" rather than "removing a
> node"
> 
> On Wed, Nov 16, 2016 at 11:35:06AM +0000, Frediano Ziglio wrote:
> > Avoid to produce loop in the tree removing and adding nodes.
> > Unlink the node from the containing tree.
> > This possibly will create unlinked trees if a parent node is
> > deleted.
> 
> If I understand this correctly, the node was only invalidated, but was
> not removed from the tree contrary to what "stat_file_remove" implies.
> This means that the could then end up being reused, which would most
> likely corrupt the tree, or something like that?
> 

Yes, what was actually happening that the creation of loops
inside the tree caused some statistical function to go into
infinite loop (and reds_stat tool too).
I don't consider the removal (which is O(n)) a performance
problem as not in a hot path.
Currently no removal is called but as we are speaking
about implementing destroying objects this would have
raised as a problem later.
To optimize we could store somewhere (for instance using
the top bits in the flag field) the previous sibling or
the parent... or we could update the format of this file
I don't think it's a big issue.

> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/stat-file.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/server/stat-file.c b/server/stat-file.c
> > index 7a07edb..54f5a1a 100644
> > --- a/server/stat-file.c
> > +++ b/server/stat-file.c
> > @@ -158,10 +158,32 @@ stat_file_add_counter(RedStatFile *stat_file,
> > StatNodeRef parent, const char *na
> >  
> >  static void stat_file_remove(RedStatFile *stat_file, SpiceStatNode *node)
> >  {
> > +    const StatNodeRef node_ref = node - stat_file->stat->nodes;
> > +    const StatNodeRef node_next = node->next_sibling_index;
> > +    StatNodeRef ref;
> > +
> >      pthread_mutex_lock(&stat_file->lock);
> >      node->flags &= ~SPICE_STAT_NODE_FLAG_ENABLED;
> >      stat_file->stat->generation++;
> >      stat_file->stat->num_of_nodes--;
> > +    /* remove links from parent or siblings */
> > +    /* childs will be orphans */
> 
> 'children' (famous mistake in libxml1 public API :)
> 
> Christophe
> > +    if (stat_file->stat->root_index == node_ref) {
> > +        stat_file->stat->root_index = node_next;
> > +    } else for (ref = 0; ref <= stat_file->max_nodes; ref++) {
> > +        node = &stat_file->stat->nodes[ref];
> > +        if (!(node->flags & SPICE_STAT_NODE_FLAG_ENABLED)) {
> > +            continue;
> > +        }
> > +        if (node->first_child_index == node_ref) {
> > +            node->first_child_index = node_next;
> > +            break;
> > +        }
> > +        if (node->next_sibling_index == node_ref) {
> > +            node->next_sibling_index = node_next;
> > +            break;
> > +        }
> > +    }
> >      pthread_mutex_unlock(&stat_file->lock);
> >  }
> >  

Frediano