[Spice-devel] [spice-server v3 1/2] utils: Introduce helpers to map channel types to names

[Frediano Ziglio]

> 
> spice_server_set_channel_security() is already mostly doing that. We can
> make its code more generic, and introduce a red_channel_get_name()
> method. This method will then be used to make debug messages more
> readable by showing the actual channel name rather than its type as
> an int.
> ---
> No changes since previous version
> 
>  server/red-channel.c |  5 +++++
>  server/red-channel.h |  2 ++
>  server/reds.c        | 33 ++++++++++++---------------------
>  server/utils.c       | 51
>  +++++++++++++++++++++++++++++++++++++++++++++++++++
>  server/utils.h       |  3 +++
>  5 files changed, 73 insertions(+), 21 deletions(-)
> 
> diff --git a/server/red-channel.c b/server/red-channel.c
> index a943023d4..c9ab64faa 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -452,6 +452,11 @@ int red_channel_is_connected(RedChannel *channel)
>      return channel && channel->priv->clients;
>  }
>  
> +const char *red_channel_get_name(RedChannel *channel)
> +{
> +    return red_channel_type_to_str(channel->priv->type);
> +}
> +
>  void red_channel_remove_client(RedChannel *channel, RedChannelClient *rcc)
>  {
>      GList *link;
> diff --git a/server/red-channel.h b/server/red-channel.h
> index 281ed0c9e..e0fd60a04 100644
> --- a/server/red-channel.h
> +++ b/server/red-channel.h
> @@ -128,6 +128,8 @@ struct RedChannelClass
>  
>  GType red_channel_get_type(void) G_GNUC_CONST;
>  
> +const char *red_channel_get_name(RedChannel *channel);
> +
>  void red_channel_add_client(RedChannel *channel, RedChannelClient *rcc);
>  void red_channel_remove_client(RedChannel *channel, RedChannelClient *rcc);
>  
> diff --git a/server/reds.c b/server/reds.c
> index 4af804065..de5f4461c 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3983,32 +3983,23 @@ SPICE_GNUC_VISIBLE int
> spice_server_set_zlib_glz_compression(SpiceServer *s, spi
>  
>  SPICE_GNUC_VISIBLE int spice_server_set_channel_security(SpiceServer *s,
>  const char *channel, int security)
>  {
> -    static const char *const names[] = {
> -        [ SPICE_CHANNEL_MAIN     ] = "main",
> -        [ SPICE_CHANNEL_DISPLAY  ] = "display",
> -        [ SPICE_CHANNEL_INPUTS   ] = "inputs",
> -        [ SPICE_CHANNEL_CURSOR   ] = "cursor",
> -        [ SPICE_CHANNEL_PLAYBACK ] = "playback",
> -        [ SPICE_CHANNEL_RECORD   ] = "record",
> -#ifdef USE_SMARTCARD
> -        [ SPICE_CHANNEL_SMARTCARD] = "smartcard",
> -#endif
> -        [ SPICE_CHANNEL_USBREDIR ] = "usbredir",
> -        [ SPICE_CHANNEL_WEBDAV ] = "webdav",
> -    };
> -    int i;
> -
> +    int type;
>      if (channel == NULL) {
>          s->config->default_channel_security = security;
>          return 0;
>      }
> -    for (i = 0; i < SPICE_N_ELEMENTS(names); i++) {
> -        if (names[i] && strcmp(names[i], channel) == 0) {
> -            reds_set_one_channel_security(s, i, security);
> -            return 0;
> -        }
> +    type = red_channel_name_to_type(channel);
> +#ifndef USE_SMARTCARD
> +    if (type == SPICE_CHANNEL_SMARTCARD) {
> +        type = -1;
>      }
> -    return -1;
> +#endif

I personally would accept just even the smartcard, maybe adding a commit
in the commit log. The only functional change to this would be accepting
"smartcard" setting even if disabled but won't be used.

Maybe a follow up.

> +    if (type == -1) {
> +        return -1;
> +    }
> +
> +    reds_set_one_channel_security(s, type, security);
> +    return 0;
>  }
>  
>  /* very obsolete and old function, retain only for ABI */
> diff --git a/server/utils.c b/server/utils.c
> index 66df86ff4..ca7e99359 100644
> --- a/server/utils.c
> +++ b/server/utils.c
> @@ -19,6 +19,7 @@
>  #endif
>  
>  #include <glib.h>
> +#include <spice/enums.h>
>  #include "utils.h"
>  
>  int rgb32_data_has_alpha(int width, int height, size_t stride,
> @@ -48,3 +49,53 @@ int rgb32_data_has_alpha(int width, int height, size_t
> stride,
>      *all_set_out = has_alpha;
>      return has_alpha;
>  }
> +

I would prefer an additional comment on not changing these
just here, as close as possible.

> +static const char *const channel_names[] = {
> +    [ SPICE_CHANNEL_MAIN     ] = "main",
> +    [ SPICE_CHANNEL_DISPLAY  ] = "display",
> +    [ SPICE_CHANNEL_INPUTS   ] = "inputs",
> +    [ SPICE_CHANNEL_CURSOR   ] = "cursor",
> +    [ SPICE_CHANNEL_PLAYBACK ] = "playback",
> +    [ SPICE_CHANNEL_RECORD   ] = "record",
> +    [ SPICE_CHANNEL_SMARTCARD] = "smartcard",
> +    [ SPICE_CHANNEL_USBREDIR ] = "usbredir",
> +    [ SPICE_CHANNEL_WEBDAV   ] = "webdav",
> +};
> +
> +/**
> + * red_channel_type_to_str:
> + *
> + * This function returns a human-readable name from a SPICE_CHANNEL_* type.
> + * It must be called with a valid channel type.
> + *
> + * Returns: NULL if the channel type is invalid, the channel name otherwise.
> + */
> +const char *red_channel_type_to_str(int type)
> +{
> +    g_return_val_if_fail(type >= 0, NULL);
> +    g_return_val_if_fail(type < G_N_ELEMENTS(channel_names), NULL);

Just for log consistency I would add

   g_return_val_if_fail(channel_names[type] != NULL, NULL);

> +
> +    return channel_names[type];
> +}
> +
> +/**
> + * red_channel_name_to_type:
> + *
> + * Converts a channel name to a SPICE_CHANNEL_* type. These names are
> currently
> + * used in our public API (see spice_server_set_channel_security()).
> + *
> + * Returns: -1 if @name was not a known channel name, the channel
> + * type otherwise.
> + *
> + */
> +int red_channel_name_to_type(const char *name)
> +{
> +    int i;
> +
> +    for (i = 0; i < G_N_ELEMENTS(channel_names); i++) {
> +        if (g_strcmp0(channel_names[i], name) == 0) {
> +            return i;
> +        }
> +    }
> +    return -1;
> +}
> diff --git a/server/utils.h b/server/utils.h
> index ec2db2c90..3f735754d 100644
> --- a/server/utils.h
> +++ b/server/utils.h
> @@ -74,4 +74,7 @@ static inline red_time_t spice_get_monotonic_time_ms(void)
>  int rgb32_data_has_alpha(int width, int height, size_t stride,
>                           uint8_t *data, int *all_set_out);
>  
> +const char *red_channel_type_to_str(int type);
> +int red_channel_name_to_type(const char *name);
> +
>  #endif /* UTILS_H_ */

Frediano