[Spice-devel] [PATCH spice-server 1/2] Refactory Glz RedDrawable retention code

[Frediano Ziglio]

> > 
> > Hey,
> > 
> > On Tue, Oct 18, 2016 at 10:28:25AM +0100, Frediano Ziglio wrote:
> > > The code was quite complicated.
> > 
> > I tried to look at this one, and it indeed is quite complicated... So
> > far, too much entangled stuff that I feel confident I'm going to make a
> > meaningful review...
> > 
> > > - a GlzEncDictImageContext was bound to a GlzDrawableInstanceItem
> > >   (1-1 relation);
> > > - multiple GlzDrawableInstanceItem were attached to RedGlzDrawable;
> > > - multiple RedGlzDrawable (one for RedClient) were attached to Drawable
> > >   using GlzImageRetention;
> > > - OOM code require all Glz dictionary used by DisplayChannel to be locked
> > >   at the same time;
> > > - drawable count computation during OOM was not corrent in case of
> > >   multiple clients;
> > > - not clear why to call glz_retention_free_drawables or
> > >   glz_retention_detach_drawables.
> > > 
> > > Now:
> > > - a GlzEncDictImageContext is bound to a GlzDictItem (still 1-1
> > > relation);
> > > - multiple GlzDictItem are attached to a Drawable using
> > > GlzImageRetention;
> > > - there is only a glz_retention_free to be called when the structure
> > >   must be destroyed.
> > 
> > By reading/parsing that, executive summary is that
> > GlzDrawableInstanceItem/RedGlzDrawable have been merged and everything
> > has been renamed.
> > 
> 
> I think this change more than that as changing structures not a kind of
> merge/rename.
> If you join wheat and eggs to do a cake you don't call the cake wheat-egg.
> That is it make no sense to have a rename patch.
> 
> When I wrote this patch I draw a diagram of the current situation
> than try to check the relationship like into a database then
> I wrote a new schema (which really liked more a entity relationship
> model than a structure model) and came to this code. To debug and
> avoid possible leaks beside the 2/2 patch I have another patch with
> some printf and counters.
> 

...

I know, this sounds a bit harsh. I think this is the third "epoch"
of this series.

On the first epoch I tried something like the merge you mentioned.
I don't remember why but after 10/15 patches was not still working
with different issues (crash, leaks, etc) and status was terrible.

So I decided to start from scratch trying to remove all the code
(with just leaks) and then try to add some code more bound to
the GLZ contexts than on drawables (in some way similar to current
one). In some way working... on the other ways (on the corner cases
like forcing OOM or multiple displays) completely wrong.

So I came with this third version, another time from scratch (and
with a better original model on the paper).

Frediano