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

[Frediano Ziglio]

> 
> 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);

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;
- display_channel_wait_for_migrate_data should be display_channel_handle_migrate_data;
- 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;
- 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.

Frediano