[Spice-devel] [PATCH] Use GByteArray to implement RedsClientMonitorsConfig

[Marc-André Lureau]

----- Original Message -----
> reds_on_main_agent_monitor_config() is manually implementing
> a grow-on-append char *. glib's GByteArray can do this for us,
> using it makes the code simpler to read.

Given that we don't have test suite, I am not really for such small cleanup changes. Also I don't clearly see the benefit in this case, realloc fits pretty well. Overall you saved only 3 lines, and use a more complex structure.

There are gazillions of places where GLib structures would help. I don't think we should start rewriting them in master.

I suggest a separate "unstable" branch for this kind of changes. I would also relax reviewing rules for it, because the effort to review that near-0 value change is big.

nack

> ---
>  server/reds-private.h |  4 +---
>  server/reds.c         | 27 +++++++++++++--------------
>  2 files changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/server/reds-private.h b/server/reds-private.h
> index 9358d27..9d8b5d1 100644
> --- a/server/reds-private.h
> +++ b/server/reds-private.h
> @@ -120,9 +120,7 @@ typedef struct SpiceCharDeviceStateItem {
>   * client, being passed to the guest */
>  typedef struct RedsClientMonitorsConfig {
>      MainChannelClient *mcc;
> -    uint8_t *buffer;
> -    int buffer_size;
> -    int buffer_pos;
> +    GByteArray *config_data;
>  } RedsClientMonitorsConfig;
>  
>  typedef struct RedsState {
> diff --git a/server/reds.c b/server/reds.c
> index 1456b75..18743a3 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -1079,10 +1079,10 @@ static void reds_client_monitors_config_cleanup(void)
>  {
>      RedsClientMonitorsConfig *cmc = &reds->client_monitors_config;
>  
> -    cmc->buffer_size = cmc->buffer_pos = 0;
> -    free(cmc->buffer);
> -    cmc->buffer = NULL;
> -    cmc->mcc = NULL;
> +    if (cmc->config_data != NULL) {
> +        g_byte_array_free(cmc->config_data, TRUE);
> +    }
> +    cmc->config_data = NULL;
>  }
>  
>  static void reds_on_main_agent_monitors_config(
> @@ -1092,19 +1092,18 @@ static void reds_on_main_agent_monitors_config(
>      VDAgentMonitorsConfig *monitors_config;
>      RedsClientMonitorsConfig *cmc = &reds->client_monitors_config;
>  
> -    cmc->buffer_size += size;
> -    cmc->buffer = realloc(cmc->buffer, cmc->buffer_size);
> -    spice_assert(cmc->buffer);
>      cmc->mcc = mcc;
> -    memcpy(cmc->buffer + cmc->buffer_pos, message, size);
> -    cmc->buffer_pos += 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\n", cmc->buffer_size);
> +    if (cmc->config_data == NULL) {
> +        cmc->config_data = g_byte_array_new();
> +    }

I guess there might be a better place for initial allocation.

> +    g_byte_array_append(cmc->config_data, message, size);
> +    msg_header = (VDAgentMessage *)cmc->config_data->data;
> +    if (sizeof(VDAgentMessage) > cmc->config_data->len ||
> +            msg_header->size > cmc->config_data->len -
> sizeof(VDAgentMessage)) {
> +        spice_debug("not enough data yet. %d\n", cmc->config_data->len);
>          return;
>      }
> -    monitors_config = (VDAgentMonitorsConfig *)(cmc->buffer +
> sizeof(*msg_header));
> +    monitors_config = (VDAgentMonitorsConfig *)(cmc->config_data->data +
> sizeof(*msg_header));
>      spice_debug("%s: %d\n", __func__, monitors_config->num_of_monitors);
>      red_dispatcher_client_monitors_config(monitors_config);
>      reds_client_monitors_config_cleanup();
> --
> 1.8.3.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>