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

[Frediano Ziglio]

> 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 ?
> 

I think so.

> >
> > 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.
> 

I sent some patches.

> > - 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 ?
> 

Yes.

> > - 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.
> 

Sent a patch event for this.

> I think you are right.
> 
> Thanks,
>      Uri.
> 
> 

Frediano