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

[Frediano Ziglio]

> 
> On 11/25/2015 02:23 PM, Frediano Ziglio wrote:
> >>
> >> 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.
> 
> This line verifies that there is only a single client.
> In other words, that migration with multi-client is not-supported,
> and also if we got here there must be a client connected.
> 
> > I don't think is so easy here to handle multiple clients and by the
> > way is not supported.
> 
> I agree, the code does not handle it yet.
> 
> >
> > The reason for the warning...
> >
> >>>       rcc = SPICE_CONTAINEROF(ring_get_head(&channel->clients),
> >>> RedChannelClient, channel_link);
> >
> > Is this! Only the first client is considered!
> 
> The first and only client is considered.
> 
> regards,
>      Uri.
> 
> 

Is this a NACK or not?

Frediano