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

[Jonathon Jongsma]

On Tue, 2015-11-03 at 19:30 +0100, Fabiano Fidêncio wrote:
> On Tue, Nov 3, 2015 at 7:10 PM, Jonathon Jongsma <jjongsma at redhat.com
> > wrote:
> > 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(DisplayChannelC
> > > > lien
> > > > 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(DisplayChannelC
> > > > lien
> > > > 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 :/
> 
> From my IRC logs:
> 13:42 <  jjongsma> It happened at startup when booting a rhel7 vm
> 13:42 <  jjongsma> after some of the startup text messages scrolled
> up
> the screen, at some point it looked like the resolution was being
> changed, and then it aborted
> 
> There was also a backtrace, but unfortunately it's not on fpaste
> anymore.
> 


Aha, Indeed. I can now reproduce on rhel 7.1 boot (though not on 7.2
nightly or rhel6 or several fedora versions). I see that this patch
actually introduces the regression in a different location because I
accidentally kept the removal of the case from release_before_push() as
well. Sorry about the foggy memory and shoddy testing. New patch coming
soon.

Jonathon