[Spice-devel] [PATCH spice-server] Avoid to use global variable for channel IDs

[Frediano Ziglio]

> 
> On Mon, Jan 30, 2017 at 06:32:57AM -0500, Frediano Ziglio wrote:
> > > 
> > > On Thu, Jan 26, 2017 at 05:58:09PM +0000, Frediano Ziglio wrote:
> > > > This patch allocate VMC IDs finding the first ID not used
> > > > instead of using a global variable and incrementing the value
> > > > for each channel created.
> > > > This solve some potential issues:
> > > > - remove the global state potentially making possible
> > > >   to use multiple SpiceServer on the same process;
> > > > - avoid to potentially overflow the variable. This can happen
> > > >   if channels are allocated/deallocated multiple times
> > > >   (currently not done by Qemu).
> > > > 
> > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > ---
> > > >  server/reds.c     | 35 +++++++++++++++++++++++++++++++++++
> > > >  server/reds.h     |  1 +
> > > >  server/spicevmc.c | 10 ++++++++--
> > > >  3 files changed, 44 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/server/reds.c b/server/reds.c
> > > > index 29485a8..44c8f4a 100644
> > > > --- a/server/reds.c
> > > > +++ b/server/reds.c
> > > > @@ -403,6 +403,41 @@ RedChannel *reds_find_channel(RedsState *reds,
> > > > uint32_t type, uint32_t id)
> > > >      return NULL;
> > > >  }
> > > >  
> > > > +/* Search for first free channel id for a specific channel type.
> > > > + * Return first id free or <0 if not found. */
> > > > +int reds_get_free_channel_id(RedsState *reds, uint32_t type)
> > > > +{
> > > > +    GListIter it;
> > > > +    RedChannel *channel;
> > > > +
> > > > +    // this mark if some IDs are used.
> > > > +    // Actually used to store boolean values but char take less memory
> > > > +    // then gboolean/bool.
> > > > +    // The size of the array limit the possible id returned but
> > > > +    // usually the IDs used for a channel type are not much.
> > > > +    char used_ids[256];
> > > > +
> > > > +    unsigned n;
> > > > +
> > > > +    // mark id used for the specific channel type
> > > > +    memset(used_ids, 0, sizeof(used_ids));
> > > > +    GLIST_FOREACH(reds->channels, it, RedChannel, channel) {
> > > > +        uint32_t this_type, this_id;
> > > > +        g_object_get(channel, "channel-type", &this_type, "id",
> > > > &this_id,
> > > > NULL);
> > > > +        if (this_type == type && this_id < SPICE_N_ELEMENTS(used_ids))
> > > > {
> > > > +            used_ids[this_id] = TRUE;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    // find first ID not marked as used
> > > > +    for (n = 0; n < SPICE_N_ELEMENTS(used_ids); ++n) {
> > > > +        if (!used_ids[n]) {
> > > > +            return n;
> > > > +        }
> > > > +    }
> > > > +    return -1;
> > > > +}
> > > > +
> > > >  static void reds_mig_cleanup(RedsState *reds)
> > > >  {
> > > >      if (reds->mig_inprogress) {
> > > > diff --git a/server/reds.h b/server/reds.h
> > > > index 28e3444..14d9071 100644
> > > > --- a/server/reds.h
> > > > +++ b/server/reds.h
> > > > @@ -48,6 +48,7 @@ uint32_t reds_get_mm_time(void);
> > > >  void reds_register_channel(RedsState *reds, RedChannel *channel);
> > > >  void reds_unregister_channel(RedsState *reds, RedChannel *channel);
> > > >  RedChannel *reds_find_channel(RedsState *reds, uint32_t type, uint32_t
> > > >  id);
> > > > +int reds_get_free_channel_id(RedsState *reds, uint32_t type);
> > > >  int reds_get_mouse_mode(RedsState *reds); // used by inputs_channel
> > > >  gboolean reds_config_get_agent_mouse(const RedsState *reds); // used
> > > >  by
> > > >  inputs_channel
> > > >  int reds_has_vdagent(RedsState *reds); // used by inputs channel
> > > > diff --git a/server/spicevmc.c b/server/spicevmc.c
> > > > index 89249b2..c184bca 100644
> > > > --- a/server/spicevmc.c
> > > > +++ b/server/spicevmc.c
> > > > @@ -224,7 +224,6 @@ red_vmc_channel_finalize(GObject *object)
> > > >  static RedVmcChannel *red_vmc_channel_new(RedsState *reds, uint8_t
> > > >  channel_type)
> > > >  {
> > > >      GType gtype = G_TYPE_NONE;
> > > > -    static uint8_t id[SPICE_END_CHANNEL] = { 0, };
> > > >  
> > > >      switch (channel_type) {
> > > >          case SPICE_CHANNEL_USBREDIR:
> > > > @@ -240,11 +239,18 @@ static RedVmcChannel
> > > > *red_vmc_channel_new(RedsState
> > > > *reds, uint8_t channel_type)
> > > >              g_error("Unsupported channel_type for
> > > >              red_vmc_channel_new():
> > > >              %u", channel_type);
> > > >              return NULL;
> > > >      }
> > > > +
> > > > +    int id = reds_get_free_channel_id(reds, channel_type);
> > > 
> > > Have you considered moving uint8_t id[SPICE_END_CHANNEL] or something
> > > similar to RedsState instead of iterating every time over all the
> > > channels to know which id to use?
> > > 
> > > Christophe
> > > 
> > 
> > Yes, but this does not solve id overflow or using the smallest number.
> 
> Is it important to use the smallest number? In case of id overflow, you
> can just fail, or fallback to the slow method. Seems a bit wasteful to
> do these iterations every time throwing away the state.
> 

In the client for QXL for instance the id is used for an index to a
statically allocated array so if it's too big it's not accepted.

> 
> > Also this function could be reused for different devices and it's
> > possible that ID is provided externally (like QXL) so scanning the
> > used one works even in this case.
> > 
> > Also these channels are created per device and giving that
> > currently there are no hot plug they are added once at start.
> > Even allowing hot plug people should not playing continuously
> > adding/removing them.
> 
> So we can just fail when there is an overflow and keep everything simple ? :)
> 

This don't solve the external (ie by Qemu) ID allocation.

> Christophe
> 

Frediano