[Spice-devel] [PATCH v3 0/9] Refactor red_channel_client_init_send_data()

[Jonathon Jongsma]

On Tue, 2016-12-20 at 05:38 -0500, Frediano Ziglio wrote:
> > 
> > 
> > A series of patches refactoring the somewhat-confusing
> > red_channel_client_init_send_data() function. The third argument to
> > this
> > function is a RedPipeItem and it was never very obvious when or why
> > we should
> > pass an item in this parameter. Sometimes callers passed NULL, and
> > sometimes
> > they passed an item. It turns out that it's only necessary to pass
> > the item
> > to
> > this parameter if the pipe item needs to be kept alive until we
> > guarantee
> > that
> > the message has been sent. We only need to do this when we have
> > marshalled
> > some
> > data by reference, and that data is owned by the RedPipeItem. To
> > make this
> > more
> > obvious, I've attempted to refactor things so that we no longer
> > rely on this
> > mechanism to keep the data alive, but instead reference the pipe
> > item (or
> > other
> > structure that owns the referenced data) call _add_by_ref_full() to
> > unreference
> > the data after the message has been sent.
> > 
> > Changes in v2:
> >  - removed one merged patch
> >  - this one compiles
> > 
> > Changes in v3:
> >  - introduce a new patch (#1) that ensures that marshaller data is
> > freed as
> > soon as possible after the message is sent. This fixes an assert
> > that
> > Frediano
> > ran into  on the original patchset because some data was kept alive
> > longer
> > than
> > expected.
> >  - Use Frediano's SET/INIT patch (4) instead of my original
> > approach
> >  - squashed a fix (removing unused struct field) from Frediano into
> > last
> >  patch.
> >  - removed another ACKed and pushed patch
> > 
> 
> I would ack the entire series beside:
> - I cannot ack my own patch;
> - I'm not again moving marshaller_free_pipe_item into red-pipe-
> item.c;
>   it's basically an utility for RedPipeItem. RedPipeItem, as the name
>   suggest are though to be put into the pipe and we know this pipe
>   it's the pipe that goes to the client so has to be marshaled.
>   Perhaps marshaller_unref_pipe_item instead of
> marshaller_free_pipe_item
>   would also make sense (I noted it calls a unref and that you
>   added also a marshaller_unref_drawable).


Yes, I forgot about this comment. I'll de-duplicate these functions and
move it to red-pipe-item.h and sent another version