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

[Uri Lublin]

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.