[Spice-devel] [PATCH 14/16] Improve Reds/RedMainChannel interface

[Jonathon Jongsma]

On Tue, 2016-04-19 at 11:00 -0500, Jonathon Jongsma wrote:
> From: Christophe Fergeau <cfergeau at redhat.com>
> 
> Instead of exporting 2 methods to get number of channels, and to fill
> channel information, and use that from the main channel code, it's
> better to do everything in one go in reds.c, and call that single method
> from the main channel code.
> ---
>  server/main-channel.c |  4 +---
>  server/reds.c         | 22 +++++++++++++++-------
>  server/reds.h         |  3 +--
>  3 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/server/main-channel.c b/server/main-channel.c
> index 33f8eed..7d55de7 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -340,9 +340,7 @@ static void
> main_channel_marshall_channels(RedChannelClient *rcc,
>      SpiceMsgChannels* channels_info;
>  
>      red_channel_client_init_send_data(rcc, SPICE_MSG_MAIN_CHANNELS_LIST,
> item);
> -    channels_info = (SpiceMsgChannels *)spice_malloc(sizeof(SpiceMsgChannels)
> -                            + reds_get_n_channels(rcc->channel->reds) *
> sizeof(SpiceChannelId));
> -    reds_fill_channels(rcc->channel->reds, channels_info);
> +    channels_info = reds_msg_channels_new(rcc->channel->reds);
>      spice_marshall_msg_main_channels_list(m, channels_info);
>      free(channels_info);
>  }
> diff --git a/server/reds.c b/server/reds.c
> index 5ee913a..556d0cb 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -977,12 +977,6 @@ void reds_handle_agent_mouse_event(RedsState *reds, const
> VDAgentMouseState *mou
>      red_char_device_write_buffer_add(RED_CHAR_DEVICE(reds->agent_dev),
> char_dev_buf);
>  }
>  
> -int reds_get_n_channels(RedsState *reds)
> -{
> -    return reds ? reds->num_of_channels : 0;
> -}
> -
> -
>  static int reds_get_n_clients(RedsState *reds)
>  {
>      return reds ? reds->num_clients : 0;
> @@ -1005,7 +999,7 @@ static int channel_supports_multiple_clients(RedChannel
> *channel)
>      return FALSE;
>  }
>  
> -void reds_fill_channels(RedsState *reds, SpiceMsgChannels *channels_info)
> +static void reds_fill_channels(RedsState *reds, SpiceMsgChannels
> *channels_info)
>  {
>      RingItem *now;
>      int used_channels = 0;
> @@ -1028,6 +1022,20 @@ void reds_fill_channels(RedsState *reds,
> SpiceMsgChannels *channels_info)
>      }
>  }
>  
> +SpiceMsgChannels *reds_msg_channels_new(RedsState *reds)
> +{
> +    SpiceMsgChannels* channels_info;
> +
> +    g_return_val_if_fail(reds != NULL, NULL);

Although it will only occur with a programming error, this NULL return could
introduce a crash that wasn't possible before. The caller does not check the
returned value from this function to make sure it's non-NULL. In that case,
maybe it's better to simply assert here so that it crashes at the source of the
issue rather than later on when the value is used.

> +
> +    channels_info = (SpiceMsgChannels *)spice_malloc(sizeof(SpiceMsgChannels)
> +                            + reds->num_of_channels *
> sizeof(SpiceChannelId));
> +
> +    reds_fill_channels(reds, channels_info);
> +
> +    return channels_info;
> +}
> +
>  void reds_on_main_agent_start(RedsState *reds, MainChannelClient *mcc,
> uint32_t num_tokens)
>  {
>      RedCharDevice *dev_state = RED_CHAR_DEVICE(reds->agent_dev);
> diff --git a/server/reds.h b/server/reds.h
> index ef1f2a8..9de506f 100644
> --- a/server/reds.h
> +++ b/server/reds.h
> @@ -79,8 +79,7 @@ void reds_client_disconnect(RedsState *reds, RedClient
> *client);
>  // Temporary (?) for splitting main channel
>  typedef struct MainMigrateData MainMigrateData;
>  void reds_marshall_migrate_data(RedsState *reds, SpiceMarshaller *m);
> -void reds_fill_channels(RedsState *reds, SpiceMsgChannels *channels_info);
> -int reds_get_n_channels(RedsState *reds);
> +SpiceMsgChannels *reds_msg_channels_new(RedsState *reds);
>  
>  /* callbacks from main channel messages */
>  

I agree that the previous code was rather awkward, but I feel that the "message"
structure is more a responsibility for the the Main Channel than of the
RedsState object. I'd probably ACK this change anyway, though.

Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>