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

[Jonathon Jongsma]

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 

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

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