[Spice-devel] [spice PATCH v1 1/3] char-device: fix usage of free/unref on WriteBuffer

[Fabiano Fidêncio]

On Thu, Nov 12, 2015 at 5:20 PM, Victor Toso <lists at victortoso.com> wrote:
> Hi,
>
> On Thu, Nov 12, 2015 at 05:12:26PM +0100, Fabiano Fidêncio wrote:
>> On Thu, Nov 12, 2015 at 5:00 PM, Victor Toso <victortoso at redhat.com> wrote:
>> > There are places were the could should definetly free the
>> > SpiceCharDeviceWriteBuffer and places that it should only unref it. The
>> > current use of spice_char_device_write_buffer_free was missleading.
>> >
>> > This patch creates the spice_char_device_write_buffer_unref and properly
>> > call these two functions.
>> >
>> > Related: https://bugs.freedesktop.org/show_bug.cgi?id=91350
>> > ---
>> >  server/char_device.c | 34 ++++++++++++++++++++++------------
>> >  1 file changed, 22 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/server/char_device.c b/server/char_device.c
>> > index 54357f0..ee52e90 100644
>> > --- a/server/char_device.c
>> > +++ b/server/char_device.c
>> > @@ -81,6 +81,7 @@ enum {
>> >   * destroyed during a callback */
>> >  static void spice_char_device_state_ref(SpiceCharDeviceState *char_dev);
>> >  static void spice_char_device_state_unref(SpiceCharDeviceState *char_dev);
>> > +static void spice_char_device_write_buffer_unref(SpiceCharDeviceWriteBuffer *write_buf);
>> >
>> >  static void spice_char_dev_write_retry(void *opaque);
>> >
>> > @@ -91,10 +92,11 @@ typedef struct SpiceCharDeviceMsgToClientItem {
>> >
>> >  static void spice_char_device_write_buffer_free(SpiceCharDeviceWriteBuffer *buf)
>> >  {
>> > -    if (--buf->refs == 0) {
>> > -        free(buf->buf);
>> > -        free(buf);
>> > -    }
>> > +    if (!buf)
>> > +        return;
>>
>> Please, keep the check explicit.
>
> Will do!
>
>>
>> > +
>> > +    free(buf->buf);
>> > +    free(buf);
>> >  }
>> >
>> >  static void write_buffers_queue_free(Ring *write_queue)
>> > @@ -117,9 +119,11 @@ static void spice_char_device_write_buffer_pool_add(SpiceCharDeviceState *dev,
>> >          buf->origin = WRITE_BUFFER_ORIGIN_NONE;
>> >          buf->client = NULL;
>> >          ring_add(&dev->write_bufs_pool, &buf->link);
>> > -    } else {
>> > -        --buf->refs;
>> > +        return;
>> >      }
>> > +
>> > +    /* Buffer still being used - just unref for the caller */
>> > +    spice_char_device_write_buffer_unref(buf);
>> >  }
>> >
>> >  static void spice_char_device_client_send_queue_free(SpiceCharDeviceState *dev,
>> > @@ -593,6 +597,15 @@ static SpiceCharDeviceWriteBuffer *spice_char_device_write_buffer_ref(SpiceCharD
>> >      return write_buf;
>> >  }
>> >
>> > +static void spice_char_device_write_buffer_unref(SpiceCharDeviceWriteBuffer *write_buf)
>> > +{
>> > +    spice_assert(write_buf);
>> > +
>> > +    write_buf->refs--;
>> > +    if (write_buf->refs == 0)
>> > +        spice_char_device_write_buffer_free(write_buf);
>>
>> Not important, but you could keep the styling:
>> if (--write_buf->refs != 0)
>>     return
>> spice_char_device_write_buffer_free(write_buf);
>
> I really don't think this way is more readeable. Do you want me to
> change it to keep the style of the code? (just confirming if it is
> necessary or not).

Not necessary as long as you keep the check explicit. :-)

>
>>
>> > +}
>> > +
>> >  void spice_char_device_write_buffer_add(SpiceCharDeviceState *dev,
>> >                                          SpiceCharDeviceWriteBuffer *write_buf)
>> >  {
>> > @@ -619,8 +632,7 @@ void spice_char_device_write_buffer_release(SpiceCharDeviceState *dev,
>> >      spice_assert(!ring_item_is_linked(&write_buf->link));
>> >      if (!dev) {
>> >          spice_printerr("no device. write buffer is freed");
>> > -        free(write_buf->buf);
>> > -        free(write_buf);
>> > +        spice_char_device_write_buffer_free(write_buf);
>> >          return;
>> >      }
>> >
>> > @@ -727,9 +739,7 @@ void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
>> >      }
>> >      write_buffers_queue_free(&char_dev->write_queue);
>> >      write_buffers_queue_free(&char_dev->write_bufs_pool);
>> > -    if (char_dev->cur_write_buf) {
>> > -        spice_char_device_write_buffer_free(char_dev->cur_write_buf);
>> > -    }
>> > +    spice_char_device_write_buffer_free(char_dev->cur_write_buf);
>> >
>> >      while (!ring_is_empty(&char_dev->clients)) {
>> >          RingItem *item = ring_get_tail(&char_dev->clients);
>> > @@ -895,7 +905,7 @@ static void migrate_data_marshaller_write_buffer_free(uint8_t *data, void *opaqu
>> >  {
>> >      SpiceCharDeviceWriteBuffer *write_buf = (SpiceCharDeviceWriteBuffer *)opaque;
>> >
>> > -    spice_char_device_write_buffer_free(write_buf);
>> > +    spice_char_device_write_buffer_unref(write_buf);
>> >  }
>> >
>> >  void spice_char_device_state_migrate_data_marshall(SpiceCharDeviceState *dev,
>> > --
>> > 2.5.0
>> >
>> > _______________________________________________
>> > Spice-devel mailing list
>> > Spice-devel at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
>> Looks good, ACK!
>
> Thanks,
>   toso