On Tue, Nov 3, 2015 at 7:10 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:
> On Tue, 2015-11-03 at 12:16 -0500, Frediano Ziglio wrote:
>> >
>> > From: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
>> >
>> > 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel