>
> On Tue, Jan 24, 2017 at 07:26:15AM -0500, Frediano Ziglio wrote:
> > >
> > > On Tue, Jan 10, 2017 at 03:59:38PM +0000, Frediano Ziglio wrote:
> > > > Code to read and process display commands were the same
> > > > so use a common function for better reuse.
> > > >
> > > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > > > ---
> > > > server/red-worker.c | 38 ++++++++++++++++++--------------------
> > > > 1 file changed, 18 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/server/red-worker.c b/server/red-worker.c
> > > > index 394a935..49ec8e8 100644
> > > > --- a/server/red-worker.c
> > > > +++ b/server/red-worker.c
> > > > @@ -169,6 +169,19 @@ static RedDrawable *red_drawable_new(QXLInstance
> > > > *qxl)
> > > > return red;
> > > > }
> > > >
> > > > +static gboolean red_process_surface_cmd(RedWorker *worker,
> > > > QXLCommandExt
> > > > *ext, gboolean loadvm)
> > > > +{
> > > > + RedSurfaceCmd surface_cmd;
> > > > +
> > > > + if (red_get_surface_cmd(&worker->mem_slots, ext->group_id,
> > > > &surface_cmd, ext->cmd.data)) {
> > > > + return FALSE;
> > > > + }
> > > > + display_channel_process_surface_cmd(worker->display_channel,
> > > > &surface_cmd, loadvm);
> > > > + // do not release resource as is released inside
> > > > display_channel_process_surface_cmd
> > >
> > > I'd use this as an opportunity to improve that comment to something
> > > like:
> > >
> > > // do not release resource ('release_info_ext') as it will be released
> > > inside
> > > // display_channel_surface_unref() once the last reference is dropped
> > >
> >
> > I think this comment is half an improvement and half worsening.
> > The improvement is "('release_info_ext')", the worsening
> > display_channel_surface_unref.
> > Is up to display_channel_process_surface_cmd how to release the resource,
> > for instance can free resource directly on double create or surface too
> > big.
> >
> > Perhaps
> >
> > // do not release resource ('release_info_ext') as
> > // display_channel_process_surface_cmd() will take care of
>
> Ah, different perspective on this. I think what you want to say is that
> display_channel_process_surface_cmd() will take ownership of the
> resource (ie we no longer need to care about it).
>
> My point was more along the line that if you look at
> display_channel_process_surface_cmd() code, there is nothing releasing
This is wrong.
Just you should not look at how display_channel_process_surface_cmd is
implemented, this is up to DisplayChannel object, not red worker.
Unfortunately you cannot declare ownership in C so this should be
documented for display_channel_process_surface_cmd (as the comment does,
perhaps would be worth adding a comment in DisplayChannel header/code).
> resources, so the code aluded to in the comment does not exist there!
> They are only released once the last reference is dropped, in
> display_channel_surface_unref().
>
> So I'm fine with something like "display_channel_process_surface_cmd()
> takes ownership of 'release_info_ext', we don't need to
> release it ourselves"
>
Yes, this is fine.
> Christophe
>
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel