Re: [PATCH 03/10] Fix warning due to unexpected pipe item type

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 2015-11-02 at 10:55 -0500, Frediano Ziglio wrote:
> > 
> > On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio <
> > fziglio@xxxxxxxxxx> wrote:
> > > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> > > 
> > > The specific item type that was not being handled was
> > > PIPE_ITEM_TYPE_INVAL_ONE (#102). This item type is used by the
> > > cursor
> > > channel, but the analogous item for the display channel is
> > > PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE. Use this value instead.
> > > 
> > > The exact warning follows:
> > > 
> > >      (/usr/bin/qemu-kvm:24458): Spice-Warning **:
> > >      ../../server/dcc-send.c:2442:dcc_send_item: should not be
> > > reached
> > >      (/usr/bin/qemu-kvm:24458): Spice-CRITICAL **:
> > >      ../../server/dcc.c:1595:release_item_before_push: invalid
> > > item type
> > > 
> > > Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> > > ---
> > >  server/cache_item.tmpl.c |  4 +++-
> > >  server/red_worker.c      | 15 ---------------
> > >  2 files changed, 3 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/server/cache_item.tmpl.c b/server/cache_item.tmpl.c
> > > index dc314c0..ad2b579 100644
> > > --- a/server/cache_item.tmpl.c
> > > +++ b/server/cache_item.tmpl.c
> > > @@ -21,6 +21,7 @@
> > >  #define CACHE_HASH_KEY CURSOR_CACHE_HASH_KEY
> > >  #define CACHE_HASH_SIZE CURSOR_CACHE_HASH_SIZE
> > >  #define CACHE_INVAL_TYPE SPICE_MSG_CURSOR_INVAL_ONE
> > > +#define PIPE_ITEM_TYPE PIPE_ITEM_TYPE_INVAL_ONE
> > >  #define FUNC_NAME(name) red_cursor_cache_##name
> > >  #define VAR_NAME(name) cursor_cache_##name
> > >  #define CHANNEL CursorChannel
> > > @@ -32,6 +33,7 @@
> > >  #define CACHE_HASH_KEY PALETTE_CACHE_HASH_KEY
> > >  #define CACHE_HASH_SIZE PALETTE_CACHE_HASH_SIZE
> > >  #define CACHE_INVAL_TYPE SPICE_MSG_DISPLAY_INVAL_PALETTE
> > > +#define PIPE_ITEM_TYPE PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE
> > >  #define FUNC_NAME(name) red_palette_cache_##name
> > >  #define VAR_NAME(name) palette_cache_##name
> > >  #define CHANNEL DisplayChannel
> > > @@ -78,7 +80,7 @@ static void FUNC_NAME(remove)(CHANNELCLIENT
> > > *channel_client, CacheItem *item)
> > >      channel_client->VAR_NAME(items)--;
> > >      channel_client->VAR_NAME(available) += item->size;
> > > 
> > > -    red_channel_pipe_item_init(&channel->common.base, &item
> > > ->u.pipe_data,
> > > PIPE_ITEM_TYPE_INVAL_ONE);
> > > +    red_channel_pipe_item_init(&channel->common.base, &item
> > > ->u.pipe_data,
> > > PIPE_ITEM_TYPE);
> > >      red_channel_client_pipe_add_tail(&channel_client
> > > ->common.base,
> > >      &item->u.pipe_data); // for now
> > >  }
> > > 
> > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > index 89cb25c..93a305a 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -7805,17 +7805,6 @@ 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)
> > > -{
> > > -    SpiceMsgDisplayInvalOne inval_one;
> > > -
> > > -    red_channel_client_init_send_data(rcc, cach_item
> > > ->inval_type, NULL);
> > > -    inval_one.id = *(uint64_t *)&cach_item->id;
> > > -
> > > -    spice_marshall_msg_cursor_inval_one(base_marshaller,
> > > &inval_one);
> > > -}
> > > -
> > >  static void
> > >  display_channel_marshall_migrate_data_surfaces(DisplayChannelCli
> > > ent *dcc,
> > >                                                            
> > >  SpiceMarshaller
> > >                                                             *m,
> > >                                                             int
> > > lossy)
> > > @@ -8271,9 +8260,6 @@ static void
> > > display_channel_send_item(RedChannelClient *rcc, PipeItem
> > > *pipe_item
> > >          marshall_qxl_drawable(rcc, m, dpi);
> > >          break;
> > >      }
> > > -    case PIPE_ITEM_TYPE_INVAL_ONE:
> > > -        red_marshall_inval(rcc, m, (CacheItem *)pipe_item);
> > > -        break;
> > >      case PIPE_ITEM_TYPE_STREAM_CREATE: {
> > >          StreamAgent *agent = SPICE_CONTAINEROF(pipe_item,
> > > StreamAgent,
> > >          create_item);
> > >          red_display_marshall_stream_start(rcc, m, agent);
> > > @@ -9580,7 +9566,6 @@ static void
> > > display_channel_client_release_item_before_push(DisplayChannelCli
> > > ent
> > >          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:
> > > --
> > > 2.4.3
> > > 
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 
> > ACK!
> > 
> 
> If I remember this patch is still on the list cause form a comment
> from
> Christophe (agreed by Jonathon) this patch should be merged to
> another.
> Jonathon agreed to do some testing.
> 


Not testing, exactly. More like investigation. I now think I understand
what's going on here.

It looks like this part of the patch was not a part of the original
patch on alon's replay branch: http://cgit.freedesktop.org/~alon/spice/
commit/?h=wip/replay&id=ab2977d0bf1d71a55b58ebc61493b300af380655. But
the original patch did remove the PIPE_ITEM_TYPE_INVAL_ONE case from
display_channel_send_item(). Since cache_item.tmpl.c file still sends a
message with this type in the _remove() function (even for the display
channel), a warning was introduced when display channel no longer
handled this message type.

I'm starting to realize that it was probably me who added this fix. I
probably noticed the warning while working on a later point of this
branch. After bisecting the branch, I probably realized that the above
commit introduced this warning and then squashed a fix into that commit
(along with the confusing commit message that referred to a file and a
function that doesn't exist at this point in the branch...).

But as Christophe suggests, my fix appears to be incorrect since we are
invalidating the entire cache rather than just a single cache item.
Looking at the spice protocol definition, the display channel has an
'inval_palette' message, and an 'inval_all_palettes' message. I
presumably didn't look closely enough and thought that
PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE mapped to the 'inval_palette'
message. But it actually maps to the 'inval_all_palettes' message. I'll
try to send an updated patch to fix this issue soon.

Jonathon



_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]