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

[Frediano Ziglio]

> 
> 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.
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.

Frediano

> > +    if (id < 0) {
> > +        g_warning("Free ID not found creating new VMC channel");
> > +        return NULL;
> > +    }
> > +
> >      return g_object_new(gtype,
> >                          "spice-server", reds,
> >                          "core-interface", reds_get_core_interface(reds),
> >                          "channel-type", channel_type,
> > -                        "id", id[channel_type]++,
> > +                        "id", id,
> >                          "handle-acks", FALSE,
> >                          "migration-flags",
> >                          (SPICE_MIGRATE_NEED_FLUSH |
> >                          SPICE_MIGRATE_NEED_DATA_TRANSFER),