[Spice-devel] [PATCH 13/18] display: replace some dubious asserts

[Frediano Ziglio]

> 
> On Mon, 2015-11-23 at 17:01 +0000, Frediano Ziglio wrote:
> > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > 
> > Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
> 
> As I said in my previous review, I think that using g_return_if_fail() here
> is
> appropriate.
> 
> 

Yes, actually the only caller ignore the result so false or true is the same.
Also there is the same check outside so this path is never taken.
Basically the code is handling messages for migration till migration is ended.
red_channel_client_waits_for_migrate_data just returns if we are in a migration
(TRUE) or not (FALSE).

> 
> > ---
> >  server/display-channel.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 5e75019..a178cc9 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -861,10 +861,10 @@ int
> > display_channel_wait_for_migrate_data(DisplayChannel
> > *display)
> >      RedChannelClient *rcc;
> >  
> >      spice_debug(NULL);
> > -    spice_assert(channel->clients_num == 1);
> > +    spice_warn_if_fail(channel->clients_num == 1);
> >  

I agree with this warning but here I'd like to see a TODO also.

I won't kill the VM just because we have a single client.
I don't think is so easy here to handle multiple clients and by the
way is not supported.

The reason for the warning...

> >      rcc = SPICE_CONTAINEROF(ring_get_head(&channel->clients),
> > RedChannelClient, channel_link);

Is this! Only the first client is considered!

> > -    spice_assert(red_channel_client_waits_for_migrate_data(rcc));
> > +
> > spice_return_val_if_fail(red_channel_client_waits_for_migrate_data(rcc),
> > FALSE);
> >  
> >      for (;;) {
> >          red_channel_client_receive(rcc);
> 

I think that the right think to do is not assert/return_if_fail but:
- remove the check for red_channel_client_waits_for_migrate_data from the caller;
- do the check here and if FALSE return FALSE, no critical/warning/error to report.

Frediano