[Spice-devel] [PATCH spice-server 3/3] red-worker: Reuse code to process display command

[Christophe Fergeau]

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 at redhat.com>
> > > ---
> > >  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
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"

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170124/35f3fdaa/attachment.sig>