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

[Frediano Ziglio]

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

I moved the red_channel_waits_for_migrate_data test from external caller
to this function. This function already check if there are at least one
client waiting for migration data.
So the warning test if there are more then one clients.

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

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