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

[Frediano Ziglio]

> 
> On Tue, 2016-10-18 at 10:28 +0100, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/image-encoders.c | 27 ++++++++++++++++++++++++++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git a/server/image-encoders.c b/server/image-encoders.c
> > index 951fe96..70eb24a 100644
> > --- a/server/image-encoders.c
> > +++ b/server/image-encoders.c
> > @@ -20,6 +20,7 @@
> >  #endif
> >  
> >  #include <glib.h>
> > +#include <common/log.h>
> >  
> >  #include "image-encoders.h"
> >  #include "spice-bitmap-utils.h"
> > @@ -65,6 +66,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);
> I prefer explicit comparison to NULL (just a personal preference)
> 
> > +    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);
> like you did here, but it is not covered in spice style
> 
> > +    }
> > +}
> > +#else
> > +static inline void glz_dict_item_check(GlzDictItem *glz_item)
>                                  G_GNUC_UNUSED    ^

Missed this :(
Posting a new version.

> > +{
> > +}
> > +#endif
> >  
> >  static SPICE_GNUC_NORETURN SPICE_GNUC_PRINTF(2, 3) void
> >  quic_usr_error(QuicUsrContext *usr, const char *fmt, ...)
> > @@ -370,6 +388,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);
> > @@ -385,6 +404,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);
> >      }
> >  }
> >  
> > @@ -486,7 +506,7 @@ static gboolean glz_context_free(GlzDictItem
> > *glz_item)
> >  {
> >      gboolean ret;
> >  
> > -    spice_assert(glz_item);
> > +    glz_dict_item_check(glz_item);
> 
> Here you actually remove the check. What about defining the less
> strict variant of the glz_dict_item_check as:
> +#else
> +static inline void glz_dict_item_check(GlzDictItem *glz_item)
> +{
> +    spice_assert(glz_item != NULL);
> +}
> +#endif
> 
> Pavel

This would add lot of useless checks which was not the intention
of this patch.

> >  
> >      ring_remove(&glz_item->glz_context_link);
> >      ring_remove(&glz_item->retention_link);
> > @@ -528,6 +548,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;
> >          }
> > @@ -546,6 +567,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);
> > @@ -566,6 +588,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);
> > @@ -581,6 +604,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);
> >      }
> > @@ -1091,6 +1115,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;
> >  }
> >  
> 

Frediano