[Spice-devel] [PATCH 01/15] Palette cache: Use correct marshal function

[Jonathon Jongsma]

On Tue, 2015-11-03 at 12:16 -0500, Frediano Ziglio wrote:
> > 
> > From: Jonathon Jongsma <jjongsma at redhat.com>
> > 
> > In order to invalidate a single palette cache item, we were using
> > spice_marshall_msg_cursor_inval_one(), which is the marshal
> > function
> > used to send an invalidation message for the Cursor channel's
> > cache.
> > This didn't cause any problems because SPICE_MSG_CURSOR_INVAL_ONE
> > and
> > SPICE_MSG_DISPLAY_INVAL_PALETTE have the same message ID and
> > parameters,
> > but it's better to use the correct marshalling function.
> > ---
> >  server/red_worker.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index a15d5b6..9702652 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -7625,15 +7625,17 @@ static inline void
> > marshall_qxl_drawable(RedChannelClient *rcc,
> >          red_lossy_marshall_qxl_drawable(display_channel
> > ->common.worker, rcc,
> >          m, dpi);
> >  }
> >  
> > -static inline void red_marshall_inval(RedChannelClient *rcc,
> > -                                      SpiceMarshaller
> > *base_marshaller,
> > CacheItem *cach_item)
> > +static inline void red_marshall_inval_palette(RedChannelClient
> > *rcc,
> > +                                              SpiceMarshaller
> > *base_marshaller,
> > +                                              CacheItem
> > *cache_item)
> >  {
> >      SpiceMsgDisplayInvalOne inval_one;
> >  
> > -    red_channel_client_init_send_data(rcc, cach_item->inval_type,
> > NULL);
> > -    inval_one.id = *(uint64_t *)&cach_item->id;
> > +    red_channel_client_init_send_data(rcc, cache_item->inval_type,
> > NULL);
> > +    inval_one.id = *(uint64_t *)&cache_item->id;
> > +
> > +    spice_marshall_msg_display_inval_palette(base_marshaller,
> > &inval_one);
> >  
> > -    spice_marshall_msg_cursor_inval_one(base_marshaller,
> > &inval_one);
> >  }
> >  
> >  static void
> >  display_channel_marshall_migrate_data_surfaces(DisplayChannelClien
> > t *dcc,
> > @@ -8092,7 +8094,7 @@ static void
> > display_channel_send_item(RedChannelClient
> > *rcc, PipeItem *pipe_item
> >          break;
> >      }
> >      case PIPE_ITEM_TYPE_INVAL_ONE:
> > -        red_marshall_inval(rcc, m, (CacheItem *)pipe_item);
> > +        red_marshall_inval_palette(rcc, m, (CacheItem
> > *)pipe_item);
> >          break;
> >      case PIPE_ITEM_TYPE_STREAM_CREATE: {
> >          StreamAgent *agent = SPICE_CONTAINEROF(pipe_item,
> > StreamAgent,
> >          create_item);
> > @@ -9348,7 +9350,6 @@ static void
> > display_channel_client_release_item_before_push(DisplayChannelClien
> > t
> >          free(item);
> >          break;
> >      }
> > -    case PIPE_ITEM_TYPE_INVAL_ONE:
> >      case PIPE_ITEM_TYPE_VERB:
> >      case PIPE_ITEM_TYPE_MIGRATE_DATA:
> >      case PIPE_ITEM_TYPE_PIXMAP_SYNC:
> 
> Jonathon, did you test this issue?
> How?
> 

A little bit, but not very successfully. I'm afraid I wasn't even able
to trigger this message type. I'm not quite sure what I was doing to
trigger it when I initially discovered the issue.

As I mentioned in a previous message, the warning was caused by the
removal of the PIPE_ITEM_TYPE_INVAL_ONE case from
display_channel_send_item(). In the process of splitting the old patch,
the removal of this case was put into the same commit ("Fix warning due
to unexpected pipe item type") as the improper fix. In other words,
that patch introduced the regression and "fixed" it at the same time.
So if we simply drop that patch, the regression will not be introduced.

However, while investigating the issue, I realized that we were
technically using the wrong marshal function, so I thought it was worth
posting a new patch for that.


> It works correctly at least normal usage but I think this involve
> some migration.

I'm pretty sure that I was not doing anything related to migration when
I originally discovered the warning, but unfortunately I can't remember
the details :/


> 
> I'll add a TODO to my list. All this confusions came from bad
> marshalling
> names (spice-protocols I think). However is not a regression.
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel