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

[Fabiano Fidêncio]

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(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 :/

>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.

Best Regards,
--
Fabiano Fidêncio