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

[Uri Lublin]

On 12/02/2015 02:02 PM, Frediano Ziglio wrote:
>>
>> On 11/27/2015 11:54 AM, Frediano Ziglio wrote:
>>>>
>>> What about:
>>>
>>>
>>> display: move checks inside display_channel_wait_for_migrate_data
>>>
>>> Instead of relaying on the caller to use red_channel_waits_for_migrate_data
>>> to check if there are client waiting for migration data check inside the
>>> function. This make sure that the check is done so some check can be
>>> removed
>>> from the function.
>>>
>>>
>>> Frediano
>>>
>>
>> Hi Frediano,
>>
>> In this specific case the asserts' conditions were already tested by the
>> (only) calling function. If someone decides to call this function
>> without verifying those conditions that would be an error we want
>> to discover early. Them being there also act as a hint for the reader
>> that those conditions are assumed to be true (although that's
>> also true for spice_return_if lines).
>>
>> Your solution works, as it moves the responsibility of the checks
>> within the function. Although you may still be inclined to verify
>> (assert) that those conditions are true before proceeding.
>>
>> Personally I do not mind having those asserts in the code.
>> If people prefer, we can also return_if_fail and let the
>> calling functions deal with failures (which it currently
>> does not, as it checks those conditions itself).
>> So in the patch we can replace spice_warn_if_fail with
>> spice_return_val_if_fail.
>> Also add a commit message saying that those checks
>> are being tested by the calling function and that the calling
>> function needs to handle failures.
>
> Hi Uri,
>    with last version of patch the caller does not have to do nothing
> as the call to red_channel_waits_for_migrate_data was moved inside
> display_channel_wait_for_migrate_data. This make sure the state
> is as expected. This also assure there is at least one client
> connected. So
>
>     spice_warn_if_fail(channel->clients_num == 1);

OK.
Is spice_warn better than spice_assert here ?

>
> is just checking that there are more clients as the loop does not
> handle this case.
>
>>
>> Regards,
>>       Uri.
>>
>
> OT: the more look at this patch the more I don't like this function:
> - red_channel_waits_for_migrate_data should be red_channel_is_waiting_for_migrate_data;
I agree.

> - display_channel_wait_for_migrate_data should be display_channel_handle_migrate_data;
OK, although it both waits (by sleeping) and handles.

> - code should use loop code and not do polling and relaying on red_channel_client_receive
>    being not blocking. This is actually true for all usleep(DISPLAY_CLIENT_RETRY_INTERVAL)
>    calls;

I'm not sure what you mean. Do you mean using events/callbacks
instead of polling/sleeping ?

> - there is the wrong assumption that pointers got from channel->clients are safe
>    to survive all data processing but this is not true. We should increment the
>    reference before the loop and decrement before exiting the function.

I think you are right.

Thanks,
     Uri.