[Spice-devel] [PATCH v2 2/2] Add some debugging for GlzDictItem structure

[Pavel Grunt]

On Wed, 2016-06-29 at 11:42 -0400, Frediano Ziglio wrote:
> > 
> > 
> > Hi Frediano,
> > 
> > On Wed, 2016-06-22 at 16:19 +0100, Frediano Ziglio wrote:
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  server/image-encoders.c | 29 ++++++++++++++++++++++++++++-
> > >  1 file changed, 28 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/server/image-encoders.c b/server/image-encoders.c
> > > index 0ade8c8..c2d7648 100644
> > > --- a/server/image-encoders.c
> > > +++ b/server/image-encoders.c
> > > @@ -20,12 +20,15 @@
> > >  #endif
> > >  
> > >  #include <glib.h>
> > > +#include <common/log.h>
> > >  
> > >  #include "image-encoders.h"
> > >  #include "spice-bitmap-utils.h"
> > >  #include "red-worker.h" // red_drawable_unref
> > >  #include "pixmap-cache.h" // MAX_CACHE_CLIENTS
> > >  
> > > +#define GLZ_DEBUG 0
> > what about not defining GLZ_DEBUG, that way one could simply enable these
> > asserts by compiling with -DGLZ_DEBUG ?
> > 
> > Pavel
> > 
> 
> Make perfect sense!
> 
> We should document somewhere all that macros to turn some part of
> debugging.

We can document them & show their usage in some script. I was thinking about
having a script for "compile" testing these defines and their combinations (in
the past we were breaking them) like:
 https://paste.fedoraproject.org/386571/14672756

> One related question is in
> https://lists.freedesktop.org/archives/spice-devel/2016-May/029519.html
> 

Well I think it is ok to have a configure option, or a define like for these
debugs. imo a configure option is easier to use

Pavel



> Frediano
> 
> > 
> > > 
> > > +
> > >  #define ZLIB_DEFAULT_COMPRESSION_LEVEL 3
> > >  
> > >  #define ENCODER_MESSAGE_SIZE 512
> > > @@ -66,6 +69,23 @@ static void encoder_data_init(EncoderData *data);
> > >  static void encoder_data_reset(EncoderData *data);
> > >  static void image_encoders_release_glz(ImageEncoders *enc);
> > >  
> > > +#if GLZ_DEBUG
> > > +static void glz_dict_item_check(GlzDictItem *glz_item)
> > > +{
> > > +    spice_assert(glz_item);
> > > +    spice_assert(ring_item_is_linked(&glz_item->glz_context_link));
> > > +    spice_assert(ring_item_is_linked(&glz_item->retention_link));
> > > +    spice_assert(glz_item->enc);
> > > +    spice_assert(glz_item->red_drawable);
> > > +    if (ring_item_is_linked(&glz_item->free_link)) {
> > > +        spice_assert(glz_item->dict_image_context == NULL);
> > > +    }
> > > +}
> > > +#else
> > > +static inline void glz_dict_item_check(GlzDictItem *glz_item)
> > mark parameter as unused
> > > 
> > > +{
> > > +}
> > > +#endif
> > >  
> > >  static SPICE_GNUC_NORETURN SPICE_GNUC_PRINTF(2, 3) void
> > >  quic_usr_error(QuicUsrContext *usr, const char *fmt, ...)
> > > @@ -371,6 +391,7 @@ static void glz_usr_free_image(GlzEncoderUsrContext
> > > *usr,
> > > GlzUsrImageContext *im
> > >      GlzDictItem *glz_item = (GlzDictItem *)image;
> > >      ImageEncoders *drawable_enc = glz_item->enc;
> > >      ImageEncoders *this_enc = SPICE_CONTAINEROF(lz_data, ImageEncoders,
> > > glz_data);
> > > +    glz_dict_item_check(glz_item);
> > >      glz_item->dict_image_context = NULL;
> > >      if (this_enc == drawable_enc) {
> > >          glz_context_free(glz_item);
> > > @@ -386,6 +407,7 @@ static void glz_usr_free_image(GlzEncoderUsrContext
> > > *usr,
> > > GlzUsrImageContext *im
> > >          ring_add_before(&glz_item->free_link,
> > >                          &drawable_enc->glz_dict_items_to_free);
> > >          pthread_mutex_unlock(&drawable_enc->glz_dict_items_to_free_lock);
> > > +        glz_dict_item_check(glz_item);
> > >      }
> > >  }
> > >  
> > > @@ -487,7 +509,7 @@ static gboolean glz_context_free(GlzDictItem
> > > *glz_item)
> > >  {
> > >      gboolean ret;
> > >  
> > > -    spice_assert(glz_item);
> > > +    glz_dict_item_check(glz_item);
> > >  
> > >      ring_remove(&glz_item->glz_context_link);
> > >      ring_remove(&glz_item->retention_link);
> > > @@ -529,6 +551,7 @@ int
> > > image_encoders_free_some_independent_glz_drawables(ImageEncoders *enc, int
> > > m
> > >      while (max_to_free > n
> > >             && (ring_link = ring_get_head(&enc->glz_orphans)) != NULL) {
> > >          GlzDictItem *glz_item = SPICE_CONTAINEROF(ring_link, GlzDictItem,
> > > retention_link);
> > > +        glz_dict_item_check(glz_item);
> > >          if (glz_context_free(glz_item)) {
> > >              ++n;
> > >          }
> > > @@ -547,6 +570,7 @@ void
> > > image_encoders_free_glz_drawables_to_free(ImageEncoders* enc)
> > >      pthread_mutex_lock(&enc->glz_dict_items_to_free_lock);
> > >      while ((ring_link = ring_get_head(&enc->glz_dict_items_to_free))) {
> > >          GlzDictItem * glz_item = SPICE_CONTAINEROF(ring_link,
> > > GlzDictItem,
> > > free_link);
> > > +        glz_dict_item_check(glz_item);
> > >          glz_context_free(glz_item);
> > >      }
> > >      pthread_mutex_unlock(&enc->glz_dict_items_to_free_lock);
> > > @@ -567,6 +591,7 @@ void image_encoders_free_glz_drawables(ImageEncoders
> > > *enc)
> > >      pthread_rwlock_wrlock(&glz_dict->encode_lock);
> > >      while ((ring_link = ring_get_head(&enc->glz_dict_items))) {
> > >          GlzDictItem *glz_item = SPICE_CONTAINEROF(ring_link, GlzDictItem,
> > > glz_context_link);
> > > +        glz_dict_item_check(glz_item);
> > >          // no need to lock the to_free list, since we assured no other
> > >          thread
> > > is encoding and
> > >          // thus not other thread access the to_free list of the channel
> > >          glz_context_free(glz_item);
> > > @@ -582,6 +607,7 @@ void glz_retention_destroy(GlzImageRetention
> > > *retention)
> > >      while ((head = ring_get_head(&retention->ring)) != NULL) {
> > >          GlzDictItem *glz_item = SPICE_CONTAINEROF(head, GlzDictItem,
> > > retention_link);
> > >  
> > > +        glz_dict_item_check(glz_item);
> > >          ring_remove(head);
> > >          ring_add_before(head, &glz_item->enc->glz_orphans);
> > >      }
> > > @@ -1094,6 +1120,7 @@ static GlzDictItem *get_glz_context(ImageEncoders
> > > *enc,
> > > RedDrawable *red_drawabl
> > >  
> > >      ring_add_before(&ret->glz_context_link, &enc->glz_dict_items);
> > >      ring_add(&glz_retention->ring, &ret->retention_link);
> > > +    glz_dict_item_check(ret);
> > >      return ret;
> > >  }
> > >  
> >