[Spice-devel] [PATCH spice] migration: set low_bandwidth if enable_jpeg

[Yonit Halperin]

Hi,
On 04/25/2013 10:58 AM, Marc-André Lureau wrote:
> Hi
>
> ----- Mensaje original -----
>> Also forgot to add, that in general, regarding errors and asserts is
>> spice-server that are related to the connection to the client, we should
>> abort the server, but rather disconnect the client.
>
> Sure, but in this case, everything continue to work fine after the server warnings. Also, this is all server responsability in this case. It's not client fault, so it's a bit unfair to kick it out :)
>
After a while this can lead to inconsistencies. If we want to keep the 
connection, the safe way is to clean the surface_created array and make 
sure spice-gtk handle double creation of surfaces (it does it 
artificiality by pushing new surfaces to the head of the surfaces ring). 
Alternatively, I think is is a good idea to add to the protocol 
DESTROY_ALL_SURFACES.
>> For the specific assert the patch is referring to, I also believe the
>> client should be disconnected. When we hold a wrong list of surfaces
>> that were created in the client, it can lead to inconssitencies (e.g.,
>> sending rendering commands for surfaces that doesn't exist on the
>> client, or if we believe that the client holds surface X which was never
>> created, and then we really create such surface, it will probably lead
>> to another assert...)
>
> But in this case, the warning is coming after the fact that the surface was set wrong.
>
> Ie, the main issue is that we are not able to catch the error in the first place when we interpret the migration data wrongly. The warning comes after the fact, it's already too late.
>
>> On 04/25/2013 10:02 AM, Yonit Halperin wrote:
>>> Hi,
>>>
>>> Good Catch!
>>> Actually, the bandwidth is not getting evaluated after migration .
>>> I think it is better, if we replace
>>> display_channel_client_is_low_bandwidth with a boolean in the display
>>> channel client that will be set once, when the channel is created (by
>>> calling main_channel_client_is_low_bandwidth), or when the migration
>>> data is retrieved, by copying the value in
>>> migrate_data.low_bandwidth_setting.  Other calls to
>>> main_channel_client_is_low_bandwidth should be replaced with checking
>>> this boolean(there are places that call
>>> display_channel_client_is_low_bandwidth, and others that call
>>> main_channel_client_is_low_bandwidth  directly...argh....).
>
> I think I understand what you are asking for, but this is too much change that I am not familiar with. The server is spaghetti code and behaviour to me.
>
> We should first solve this abort(). And that's what this relatively simple and harmless patch does.
>
>>>
>>> On 04/25/2013 07:37 AM, Marc-André Lureau wrote:
>>>> The migration dest will use only the value of low_bandwidth to restore
>>>> the surfaces with lossy settings. But the server estimates
>>>> low_bandwidth on each connection, which might not be the same after
>>>> each migration.
>>>>
>>>> Older origin servers will still be vulnerable to assert() on destination.
>>>> To mitigate the issue, let's replace the assert with a warning.
>>>>
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=956345
>>>> ---
>>>>    server/red_worker.c | 7 +++++--
>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/server/red_worker.c b/server/red_worker.c
>>>> index 8ba8070..5c17b97 100644
>>>> --- a/server/red_worker.c
>>>> +++ b/server/red_worker.c
>>>> @@ -8770,7 +8770,10 @@ static void
>>>> display_channel_marshall_migrate_data(RedChannelClient *rcc,
>>>>                     MIGRATE_DATA_DISPLAY_MAX_CACHE_CLIENTS ==
>>>> MAX_CACHE_CLIENTS);
>>>>
>>>>        display_data.message_serial =
>>>> red_channel_client_get_message_serial(rcc);
>>>> -    display_data.low_bandwidth_setting =
>>>> display_channel_client_is_low_bandwidth(dcc);
>>>> +
>>>> +    display_data.low_bandwidth_setting =
>>>> +        display_channel_client_is_low_bandwidth(dcc) |
>>>> +        display_channel->enable_jpeg;
>>>>
>>>>        display_data.pixmap_cache_freezer =
>>>> pixmap_cache_freeze(dcc->pixmap_cache);
>>>>        display_data.pixmap_cache_id = dcc->pixmap_cache->id;
>>>> @@ -10110,7 +10113,7 @@ static void
>>>> display_channel_client_restore_surface(DisplayChannelClient *dcc, ui
>>>>    {
>>>>        /* we don't process commands till we receive the migration data,
>>>> thus,
>>>>         * we should have not sent any surface to the client. */
>>>> -    spice_assert(!dcc->surface_client_created[surface_id]);
>>>> +    spice_warn_if_fail(!dcc->surface_client_created[surface_id]);
>>>>        dcc->surface_client_created[surface_id] = TRUE;
>>>>    }
>>>>
>>>>
>>>
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>