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

[Uri Lublin]

On 11/26/2015 02:34 PM, Frediano Ziglio wrote:
>>
>> 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?

This is an example for the return/assert discussion.
We should be careful with simply replacing one with the other.

I'm pointing out that with the assert it's clear what would happen
if e.g. 2 (or more) clients are connected. But it's not easy to see what 
happens in such a scenario with _return_if or _warn_if.

You too mentioned there may be a problem with multiple clients
and that the code will only handle the first client (since
it's assumed that there can only be a single client after
the assert).

The multi-client feature is experimental anyway.

In case of num_clients==0, ring_get_head returns NULL and the
server crashes soon after.

Also as you mentioned, both those checks are checked before
calling this function in the single caller of this function
so currently both checks are irrelevant.

This is not NACK, but Ido not mind dropping this patch.

At least a better commit log explaining why it's safe to replace
those two asserts is needed.

Regards,
     Uri.