----- 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel