[Spice-devel] [PATCH spice-server] Release cursor as soon as possible

[Christophe Fergeau]

On Wed, Mar 01, 2017 at 11:54:20AM -0500, Frediano Ziglio wrote:
> > 
> > On Tue, Feb 28, 2017 at 03:20:09PM +0000, Frediano Ziglio wrote:
> > > Cursor resources (basically the shape of it) was retained till
> > > it was used however it was copied so there were no reason to not release
> > > this resource.
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  server/cursor-channel.c         | 14 ++++----------
> > >  server/cursor-channel.h         |  4 ++--
> > >  server/red-parse-qxl.c          | 10 +++++++---
> > >  server/red-parse-qxl.h          |  4 ++--
> > >  server/red-worker.c             |  5 +++--
> > >  server/tests/test-qxl-parsing.c | 26 +++++++++++++++++++++++---
> > >  6 files changed, 41 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> > > index 89cb120..5ed36df 100644
> > > --- a/server/red-parse-qxl.c
> > > +++ b/server/red-parse-qxl.c
> > > @@ -27,6 +27,7 @@
> > >  #include "red-common.h"
> > >  #include "memslot.h"
> > >  #include "red-parse-qxl.h"
> > > +#include "red-qxl.h"
> > >  
> > >  /* Max size in bytes for any data field used in a QXL command.
> > >   * This will for example be useful to prevent the guest from saturating
> > >   the
> > > @@ -1470,8 +1471,10 @@ static void red_put_cursor(SpiceCursor *red)
> > >  }
> > >  
> > >  int red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
> > > -                       RedCursorCmd *red, QXLPHYSICAL addr)
> > > +                       RedCursorCmd *red, QXLPHYSICAL addr,
> > > +                       QXLInstance *qxl_instance)
> > 
> > Not convinced about this patch, mainly I think because of the added
> > QXLInstance dependency which gets added to red-parse-qxl.h,
> > red_get_cursor_cmd() becoming odd compared to the other similar
> > functions, ...
> > 
> > I think it would work to have the red_qxl_release_resource() call in
> > red_process_cursor_cmd()?
> > 
> 
> This would be just a step before this...
> Memory management and resources are two sides of the coin.
> red-parse-qxl.c already deals with release_info information so
> adding a free is not really a big jump.
> Actually already some structures in red-parse-qxl.h has a QXLInstance
> which is initialized by red-worker.c. Maybe this is the issue, some
> structures are basically initialized half in red-parse-qxl.c and
> half in red-worker.c.

Dunno, but all the red_get_*_cmd() methods have a similar prototype, and
don't have a QXLInstance argument, changing this just for
red_get_cursor_cmd() makes it odd. Wrapping the QXLInstance together
with the command to release it more easily might make sense.

Looking through my old branches, I discovered some unfinished work doing
that /o\ Guess it's time to rebase it ;)

I'd prefer that we stick to the "step before this" (ie my suggestion)
until we can make this more consistent in red-parse-qxl.h

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/20170302/88129da9/attachment.sig>