On Thu, Nov 12, 2015 at 5:20 PM, Victor Toso <lists@xxxxxxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx >> > http://lists.freedesktop.org/mailman/listinfo/spice-devel >> >> Looks good, ACK! > > Thanks, > toso _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel