On Tue, 2017-03-07 at 03:18 -0500, Frediano Ziglio wrote:
> >
> > On Mon, 2017-03-06 at 18:22 +0000, Frediano Ziglio wrote:
> > > The default memset(0) on the node does not init it correctly.
> >
> > yeah, nice catch, INVALID_STAT_REF = ~0
> >
> > > Do so to avoid other nodes/counters appended to unwanted nodes.
> > >
> > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > > ---
> > > server/red-channel.c | 2 ++
> > > server/stat.h | 8 ++++++++
> > > 2 files changed, 10 insertions(+)
> > >
> > > diff --git a/server/red-channel.c b/server/red-channel.c
> > > index 8ae6ece..8206f4c 100644
> > > --- a/server/red-channel.c
> > > +++ b/server/red-channel.c
> > > @@ -300,6 +300,8 @@ red_channel_init(RedChannel *self)
> > > self->priv->client_cbs.connect =
> > > red_channel_client_default_connect;
> > > self->priv->client_cbs.disconnect =
> > > red_channel_client_default_disconnect;
> > > self->priv->client_cbs.migrate =
> > > red_channel_client_default_migrate;
> > > +
> > > + stat_init_node_invalid(&self->priv->stat);
> > > }
> > >
> > >
> > > diff --git a/server/stat.h b/server/stat.h
> > > index 5255efa..980fb07 100644
> > > --- a/server/stat.h
> > > +++ b/server/stat.h
> > > @@ -70,6 +70,14 @@ stat_remove_counter(SpiceServer *reds,
> > > RedStatCounter *counter)
> > > #endif /* RED_STATISTICS */
> > >
> > > static inline void
> > > +stat_init_node_invalid(RedStatNode *node)
> >
> > maybe 'invalidate' instead of 'invalid', or _set_invalid
> >
> > Also the argument can be marked as unused
>
> Yes,
>
> > > +{
> > > +#ifdef RED_STATISTICS
> > > + node->ref = INVALID_STAT_REF;
> > > +#endif
> > > +}
> > > +
> > > +static inline void
> > > stat_inc_counter(RedStatCounter counter, uint64_t value)
> > > {
> > > #ifdef RED_STATISTICS
> >
> > What about using / improving stat_init_node or stat_remove_node
> >
> > Wouldn't be easier to change the INVALID_STAT_REF define ?
> >
> >
> > Thanks,
> > Pavel
> >
> >
>
> INVALID_STAT_REF is currently part of the ABI for the file.
> Unless we use a different invalid for stat-file API
That's what I meant
> one way
> is this (less hacky than I though):
>
>
> diff --git a/server/reds.c b/server/reds.c
> index fc720a3..e910404 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -355,22 +355,23 @@ static void reds_link_free(RedLinkInfo *link)
> void stat_init_node(RedStatNode *node, SpiceServer *reds, const
> RedStatNode *parent,
> const char *name, int visible)
> {
> - StatNodeRef parent_ref = parent ? parent->ref :
> INVALID_STAT_REF;
> - node->ref = stat_file_add_node(reds->stat_file, parent_ref,
> name, visible);
> + StatNodeRef parent_ref = (parent ? parent->masked_ref : 0) ^
> INVALID_STAT_REF;
> + node->masked_ref =
> + stat_file_add_node(reds->stat_file, parent_ref, name,
> visible) ^ INVALID_STAT_REF;
> }
>
> void stat_remove_node(SpiceServer *reds, RedStatNode *node)
> {
> - if (node->ref != INVALID_STAT_REF) {
> - stat_file_remove_node(reds->stat_file, node->ref);
> - node->ref = INVALID_STAT_REF;
> + if (node->masked_ref) {
> + stat_file_remove_node(reds->stat_file, node->masked_ref ^
> INVALID_STAT_REF);
> + node->masked_ref = 0;
> }
> }
>
> void stat_init_counter(RedStatCounter *counter, SpiceServer *reds,
> const RedStatNode *parent, const char *name,
> int visible)
> {
> - StatNodeRef parent_ref = parent ? parent->ref :
> INVALID_STAT_REF;
> + StatNodeRef parent_ref = (parent ? parent->masked_ref : 0) ^
> INVALID_STAT_REF;
> counter->counter =
> stat_file_add_counter(reds->stat_file, parent_ref, name,
> visible);
> }
> diff --git a/server/stat.h b/server/stat.h
> index 5255efa..d191063 100644
> --- a/server/stat.h
> +++ b/server/stat.h
> @@ -32,7 +32,10 @@ typedef struct {
>
> typedef struct {
> #ifdef RED_STATISTICS
> - uint32_t ref;
> + /* to allow the use of 0 as default invalid value (ie memset(0)
> + * to work) this value is "masked" with INVALID_STAT_REF
> + * (which is part of file ABI so cannot be changes) */
> + uint32_t masked_ref;
> #endif
> } RedStatNode;
>
>
>
> Does it make sense?
The original patch is better / easier to understand. Can you add a
comment to red_channel_init(), why we need to invalidate the node.
Thanks,
Pavel
>
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel