[Spice-devel] [RFC PATCH spice-server v4 01/22] Avoid to use global variable for channel IDs

[Frediano Ziglio]

> 
> shortlog: "Avoid to" is not really grammatically correct. It should be
> something like "Avoid using" or "Don't use"
> 
> On Fri, 2017-08-25 at 10:53 +0100, Frediano Ziglio wrote:
> > This patch allocate VMC IDs finding the first ID not used
> 
> "allocates"
> "by finding"
> 
> > instead of using a global variable and incrementing the value
> > for each channel created.
> > This solve some potential issues:
> 
> "solves"
> 
> > - remove the global state potentially making possible
> >   to use multiple SpiceServer on the same process;
> > - avoid to potentially overflow the variable. This can happen
> 
> "avoid to" again
> 
> >   if channels are allocated/deallocated multiple times
> >   (currently not done by Qemu).
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/reds.c     | 34 ++++++++++++++++++++++++++++++++++
> >  server/reds.h     |  1 +
> >  server/spicevmc.c | 10 ++++++++--
> >  3 files changed, 43 insertions(+), 2 deletions(-)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index 84c2e466..005ead47 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -408,6 +408,40 @@ 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)
> > +{
> > +    RedChannel *channel;
> > +
> > +    // this mark if some IDs are used.
> > +    // Actually used to store boolean values but char take less
> > memory
> > +    // then gboolean/bool.
> 
> "than"
> 
> Also: this seems like unnecessary
> 

Yes, quite obsolete.

> > +    // The size of the array limit the possible id returned but
> 
> "limits"
> 
> > +    // usually the IDs used for a channel type are not much.
> > +    bool used_ids[256];
> > +
> > +    unsigned n;
> > +
> > +    // mark id used for the specific channel type
> > +    memset(used_ids, 0, sizeof(used_ids));
> > +    GLIST_FOREACH(reds->channels, 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;
> > +}
> 
> 
> So, if you create two display channels, they'll be ID #0 and #1. Then
> if the first channel was closed, you'd have only channel #1. Then you
> create another new channel and you'd have channel #1 and #0. I think
> our client would handle this fine, but it seems more likely to create
> client confusion than a simple monotonically-increasing ID scheme.
> 

See below

> > +
> >  static void reds_mig_cleanup(RedsState *reds)
> >  {
> >      if (reds->mig_inprogress) {
> > diff --git a/server/reds.h b/server/reds.h
> > index ee5a46c0..4f5fc28c 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);
> >  SpiceMouseMode 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 9305c9b4..08d9954b 100644
> > --- a/server/spicevmc.c
> > +++ b/server/spicevmc.c
> > @@ -272,7 +272,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:
> > @@ -288,11 +287,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);
> > +    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),
> 
> 
> 
> It feels a bit odd to add a function to SpiceServer that is purportedly
> a generic way to get the next free ID for a given channel type, but
> only ever use it for spicevmc channel types. We don't use it for inputs
> (always 0), main (always 0), playback (always 0), record (always 0),
> display (always equal to the qxl ID), cursor (always equal to the qxl
> ID), etc.
> 

The initial patch was written for SpiceVMC to remove a global variable.
For SpiceVMC the IDs are allocated monotonically as channels are never
deleted (unless you shutdown the VM).

In this series the patch is also used to allocate a new ID for the
streaming device. Currently there is only a channel (one monitor)
and is never destroyed (beside VM shutdown) so will be a consecutive
ID. For display we can't really use a monotonic counter without
reusing as there's a client (spice-gtk) limit of 16 IDs. Potentially
we could avoid to destroy and allocate again and reuse the already
allocated channels (with their monotonic IDs).

Is true, many channels use only a single ID but mainly because
only one is allowed. I remember there was a bug about a VM
configured with 2 devices causing issues (but I don't remember
exactly which kind of devices).
About current display and cursor yes, they are decided basically
by Qemu passing the ID (which are allocated from 0 monotonically).

Frediano