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

[Christophe Fergeau]

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

> +    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),
> -- 
> 2.9.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170130/a94ede66/attachment.sig>