[Spice-devel] [PATCH spice-server 1/7] char_device.c: add ref count for write-to-device buffers

[Hans de Goede]

Hi,

Looks good, one minor comment though, see
my remarks inline.

On 11/21/2012 08:42 PM, Yonit Halperin wrote:
> The ref count is used in order to keep buffers that were in the write
> queue and now are part of migration data, in case the char_device state
> is destroyed before we complete sending the migration data.
> ---
>   server/char_device.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
>   server/char_device.h |  1 +
>   2 files changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/server/char_device.c b/server/char_device.c
> index 141ec88..1c48719 100644
> --- a/server/char_device.c
> +++ b/server/char_device.c
> @@ -86,6 +86,14 @@ typedef struct SpiceCharDeviceMsgToClientItem {
>       SpiceCharDeviceMsgToClient *msg;
>   } SpiceCharDeviceMsgToClientItem;
>
> +static void spice_char_device_write_buffer_free(SpiceCharDeviceWriteBuffer *buf)
> +{
> +    if (--buf->refs == 0) {
> +        free(buf->buf);
> +        free(buf);
> +    }
> +}
> +
>   static void write_buffers_queue_free(Ring *write_queue)
>   {
>       while (!ring_is_empty(write_queue)) {
> @@ -94,18 +102,21 @@ static void write_buffers_queue_free(Ring *write_queue)
>
>           ring_remove(item);
>           buf = SPICE_CONTAINEROF(item, SpiceCharDeviceWriteBuffer, link);
> -        free(buf->buf);
> -        free(buf);
> +        spice_char_device_write_buffer_free(buf);
>       }
>   }
>
>   static void spice_char_device_write_buffer_pool_add(SpiceCharDeviceState *dev,
>                                                       SpiceCharDeviceWriteBuffer *buf)
>   {
> -    buf->buf_used = 0;
> -    buf->origin = WRITE_BUFFER_ORIGIN_NONE;
> -    buf->client = NULL;
> -    ring_add(&dev->write_bufs_pool, &buf->link);
> +    if (buf->refs == 1) {
> +        buf->buf_used = 0;
> +        buf->origin = WRITE_BUFFER_ORIGIN_NONE;
> +        buf->client = NULL;
> +        ring_add(&dev->write_bufs_pool, &buf->link);
> +    } else {
> +        --buf->refs;
> +    }
>   }
>
>   static void spice_char_device_client_send_queue_free(SpiceCharDeviceState *dev,
> @@ -530,6 +541,7 @@ static SpiceCharDeviceWriteBuffer *__spice_char_device_write_buffer_get(SpiceCha
>       }
>
>       ret->token_price = migrated_data_tokens ? migrated_data_tokens : 1;
> +    ret->refs = 1;
>       return ret;
>   error:
>       ring_add(&dev->write_bufs_pool, &ret->link);
> @@ -542,6 +554,14 @@ SpiceCharDeviceWriteBuffer *spice_char_device_write_buffer_get(SpiceCharDeviceSt
>   {
>      return  __spice_char_device_write_buffer_get(dev, client, size, 0);
>   }
> +
> +static void spice_char_device_write_buffer_ref(SpiceCharDeviceWriteBuffer *write_buf)
> +{
> +    spice_assert(write_buf);
> +
> +    write_buf->refs++;
> +}
> +


It is common for ref functions to return a pointer to the object they have just reffed, ie:

static SpiceCharDeviceWriteBuffer *spice_char_device_write_buffer_ref(SpiceCharDeviceWriteBuffer *write_buf)
{
     spice_assert(write_buf);

     write_buf->refs++;
     return write_buf;
}

>   void spice_char_device_write_buffer_add(SpiceCharDeviceState *dev,
>                                           SpiceCharDeviceWriteBuffer *write_buf)
>   {
> @@ -677,7 +697,7 @@ void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
>           spice_char_device_client_free(char_dev, dev_client);
>       }
>       char_dev->running = FALSE;
> -
> +
>       spice_char_device_state_unref(char_dev);
>   }
>
> @@ -826,6 +846,13 @@ void spice_char_device_state_migrate_data_marshall_empty(SpiceMarshaller *m)
>       mig_data->connected = FALSE;
>   }
>
> +static void migrate_data_marshaller_write_buffer_free(uint8_t *data, void *opaque)
> +{
> +    SpiceCharDeviceWriteBuffer *write_buf = (SpiceCharDeviceWriteBuffer *)opaque;
> +
> +    spice_char_device_write_buffer_free(write_buf);
> +}
> +
>   void spice_char_device_state_migrate_data_marshall(SpiceCharDeviceState *dev,
>                                                      SpiceMarshaller *m)
>   {
> @@ -857,8 +884,10 @@ void spice_char_device_state_migrate_data_marshall(SpiceCharDeviceState *dev,
>       if (dev->cur_write_buf) {
>           uint32_t buf_remaining = dev->cur_write_buf->buf + dev->cur_write_buf->buf_used -
>                                    dev->cur_write_buf_pos;
> -
> -        spice_marshaller_add_ref(m2, dev->cur_write_buf_pos, buf_remaining);
> +        spice_char_device_write_buffer_ref(dev->cur_write_buf);
> +        spice_marshaller_add_ref_full(m2, dev->cur_write_buf_pos, buf_remaining,
> +                                      migrate_data_marshaller_write_buffer_free,
> +                                      dev->cur_write_buf);

With the above ref change, this could be written as:

         spice_marshaller_add_ref_full(m2, dev->cur_write_buf_pos, buf_remaining,
                                       migrate_data_marshaller_write_buffer_free,
                                       spice_char_device_write_buffer_ref(dev->cur_write_buf));

Which IMHO is more readable.

>           *write_to_dev_size_ptr += buf_remaining;
>           if (dev->cur_write_buf->origin == WRITE_BUFFER_ORIGIN_CLIENT) {
>               spice_assert(dev->cur_write_buf->client == client_state->client);
> @@ -870,7 +899,9 @@ void spice_char_device_state_migrate_data_marshall(SpiceCharDeviceState *dev,
>           SpiceCharDeviceWriteBuffer *write_buf;
>
>           write_buf = SPICE_CONTAINEROF(item, SpiceCharDeviceWriteBuffer, link);
> -        spice_marshaller_add_ref(m2, write_buf->buf, write_buf->buf_used);
> +        spice_char_device_write_buffer_ref(write_buf);
> +        spice_marshaller_add_ref_full(m2, write_buf->buf, write_buf->buf_used,
> +                                      migrate_data_marshaller_write_buffer_free, write_buf);
>           *write_to_dev_size_ptr += write_buf->buf_used;
>           if (write_buf->origin == WRITE_BUFFER_ORIGIN_CLIENT) {
>               spice_assert(write_buf->client == client_state->client);

Idem.

> diff --git a/server/char_device.h b/server/char_device.h
> index d6d75e3..8bfe4ec 100644
> --- a/server/char_device.h
> +++ b/server/char_device.h
> @@ -73,6 +73,7 @@ typedef struct SpiceCharDeviceWriteBuffer {
>       uint32_t buf_size;
>       uint32_t buf_used;
>       uint32_t token_price;
> +    uint32_t refs;
>   } SpiceCharDeviceWriteBuffer;
>
>   typedef void SpiceCharDeviceMsgToClient;
>

Regards,

Hans