[Spice-devel] [PATCH 03/18] worker: remove cursor channel asserts

[Christophe Fergeau]

On Thu, Nov 19, 2015 at 11:39:51AM -0500, Frediano Ziglio wrote:
> > On Wed, Nov 18, 2015 at 5:17 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> > > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > >
> > > ---
> > >  server/cursor-channel.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > > index aafc807..794dcf3 100644
> > > --- a/server/cursor-channel.c
> > > +++ b/server/cursor-channel.c
> > > @@ -223,7 +223,7 @@ static void put_cursor_pipe_item(CursorChannelClient
> > > *ccc, CursorPipeItem *pipe_
> > >          return;
> > >      }
> > >
> > > -    spice_assert(!pipe_item_is_linked(&pipe_item->base));
> > > +    spice_return_if_fail(!pipe_item_is_linked(&pipe_item->base));
> > >
> > >      cursor_item_unref(pipe_item->cursor_item);
> > >      free(pipe_item);
> > > @@ -281,7 +281,7 @@ static void red_marshall_cursor_init(RedChannelClient
> > > *rcc, SpiceMarshaller *bas
> > >      SpiceMsgCursorInit msg;
> > >      AddBufInfo info;
> > >
> > > -    spice_assert(rcc);
> > > +    spice_return_if_fail(rcc);
> > >      cursor_channel = SPICE_CONTAINEROF(rcc->channel, CursorChannel,
> > >      common.base);
> > >
> > >      red_channel_client_init_send_data(rcc, SPICE_MSG_CURSOR_INIT, NULL);
> > > @@ -414,7 +414,7 @@ static void
> > > cursor_channel_release_item(RedChannelClient *rcc, PipeItem *item, i
> > >  {
> > >      CursorChannelClient *ccc = RCC_TO_CCC(rcc);
> > >
> > > -    spice_assert(item);
> > > +    spice_return_if_fail(item);
> > >
> > >      if (item_pushed) {
> > >          cursor_channel_client_release_item_after_push(ccc, item);
> > > --
> > > 2.4.3
> > >
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 
> > Seems okay, ACK!
> > 
> 
> I personally would NACK all these. Reasons:
> - there is no rationale whatsoever;

Agreed

> - with default setting the change does nothing;

Which is why using g_return_if_fail() or similar would be nice

> - actually these assert does not happen so I don't see the point
>   of changing them without being able to cause the issue.

If they don't happen, we should remove them then, not keep them...
If this situation might happen, but reviewing the code shows an early
return rather than an assert will be handled properly, then
g_return_if_fail() is what we should use here rather than exposing our
users to a potential VM crash when some corner case is hit.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20151120/febb45a8/attachment.sig>