[Spice-devel] [spice-server] reds: Replace RedsClientMonitorsConfig with SpiceBuffer

[Frediano Ziglio]

> 
> RedsClientMonitorsConfig duplicates what SpiceBuffer does,
> so using we can replace it with SpiceBuffer and make
> reds_on_main_agent_monitors_config() simpler.
> 
> Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> ---
>  server/reds-private.h |  8 +-------
>  server/reds.c         | 29 ++++++++---------------------
>  2 files changed, 9 insertions(+), 28 deletions(-)
> 
> diff --git a/server/reds-private.h b/server/reds-private.h
> index 915bcf6fd..f5be8eff5 100644
> --- a/server/reds-private.h
> +++ b/server/reds-private.h
> @@ -61,12 +61,6 @@ typedef struct RedsMigTargetClient {
>  
>  /* Intermediate state for on going monitors config message from a single
>   * client, being passed to the guest */

Why not removing the associated comment?
Maybe this should be moved before field declaration?

> -typedef struct RedsClientMonitorsConfig {
> -    uint8_t *buffer;
> -    int buffer_size;
> -    int buffer_pos;
> -} RedsClientMonitorsConfig;
> -
>  typedef struct ChannelSecurityOptions ChannelSecurityOptions;
>  
>  typedef struct RedSSLParameters {
> @@ -126,7 +120,7 @@ struct RedsState {
>  #endif
>      int allow_multiple_clients;
>  
> -    RedsClientMonitorsConfig client_monitors_config;
> +    SpiceBuffer client_monitors_config;
>      int mm_time_enabled;
>      uint32_t mm_time_latency;
>  
> diff --git a/server/reds.c b/server/reds.c
> index 09b674d7b..b543eacfe 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -1102,37 +1102,25 @@ void reds_release_agent_data_buffer(RedsState *reds,
> uint8_t *buf)
>      dev->priv->recv_from_client_buf_pushed = FALSE;
>  }
>  
> -static void reds_client_monitors_config_cleanup(RedsState *reds)
> -{
> -    RedsClientMonitorsConfig *cmc = &reds->client_monitors_config;
> -
> -    cmc->buffer_size = cmc->buffer_pos = 0;
> -    free(cmc->buffer);
> -    cmc->buffer = NULL;
> -}
> -
>  static void reds_on_main_agent_monitors_config(RedsState *reds,
>          MainChannelClient *mcc, const void *message, size_t size)
>  {
>      VDAgentMessage *msg_header;
>      VDAgentMonitorsConfig *monitors_config;
> -    RedsClientMonitorsConfig *cmc = &reds->client_monitors_config;
> +    SpiceBuffer *cmc;
>  
> -    cmc->buffer_size += size;
> -    cmc->buffer = realloc(cmc->buffer, cmc->buffer_size);
> -    spice_assert(cmc->buffer);
> -    memcpy(cmc->buffer + cmc->buffer_pos, message, size);
> -    cmc->buffer_pos += size;
> +    cmc = &reds->client_monitors_config;
> +    spice_buffer_append(cmc, message, size);
>      msg_header = (VDAgentMessage *)cmc->buffer;
> -    if (sizeof(VDAgentMessage) > cmc->buffer_size ||
> -            msg_header->size > cmc->buffer_size - sizeof(VDAgentMessage)) {
> -        spice_debug("not enough data yet. %d", cmc->buffer_size);
> +    if (sizeof(VDAgentMessage) > cmc->offset ||
> +            msg_header->size > cmc->offset - sizeof(VDAgentMessage)) {
> +        spice_debug("not enough data yet. %zd", cmc->offset);
>          return;
>      }
>      monitors_config = (VDAgentMonitorsConfig *)(cmc->buffer +
>      sizeof(*msg_header));
>      spice_debug("%s: %d", __func__, monitors_config->num_of_monitors);
>      reds_client_monitors_config(reds, monitors_config);
> -    reds_client_monitors_config_cleanup(reds);
> +    spice_buffer_free(cmc);
>  }
>  
>  void reds_on_main_agent_data(RedsState *reds, MainChannelClient *mcc, const
>  void *message,

Weird name "offset" for a length. Usually I found capacity + len(gth), but
is not a regression.

> @@ -3410,6 +3398,7 @@ static int do_spice_init(RedsState *reds,
> SpiceCoreInterface *core_interface)
>      reds->char_devices = NULL;
>      reds->mig_wait_disconnect_clients = NULL;
>      reds->vm_running = TRUE; /* for backward compatibility */
> +    spice_buffer_free(&reds->client_monitors_config);
>  
>      if (!(reds->mig_timer = reds->core.timer_add(&reds->core,
>      migrate_timeout, reds))) {
>          spice_error("migration timer create failed");
> @@ -3438,8 +3427,6 @@ static int do_spice_init(RedsState *reds,
> SpiceCoreInterface *core_interface)
>  
>      reds->mouse_mode = SPICE_MOUSE_MODE_SERVER;
>  
> -    reds_client_monitors_config_cleanup(reds);
> -
>      reds->allow_multiple_clients = getenv(SPICE_DEBUG_ALLOW_MC_ENV) != NULL;
>      if (reds->allow_multiple_clients) {
>          spice_warning("spice: allowing multiple client connections");

Weird to have a cleanup/free call during an init but not a regression.
Why did you move it to the start of the function?
Maybe should also be called by spice_server_destroy.

Beside these patch looks good (I'll test it tomorrow).

Frediano