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

Submitted by Frediano Ziglio on Nov. 3, 2015, 10:20 a.m.

Details

Message ID 1446546023-7986-2-git-send-email-fziglio@redhat.com
State Superseded
Headers show
Series "Backported some patches from refactory branches (3rd Nov)" ( rev: 3 2 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Nov. 3, 2015, 10:20 a.m.
From: Jonathon Jongsma <jjongsma@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(-)

Patch hide | download patch | download mbox

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(DisplayChannelClient *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(DisplayChannelClient
         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:

Comments

> 
> From: Jonathon Jongsma <jjongsma@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(DisplayChannelClient *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(DisplayChannelClient
>          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?

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

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