[Spice-devel] [PATCH 5/7] Don't increment num_clients_mig_wait twice

[Uri Lublin]

On 10/12/2016 05:19 PM, Pavel Grunt wrote:
> On Wed, 2016-10-12 at 10:13 -0400, Frediano Ziglio wrote:
>>>
>>> On Wed, 2016-10-12 at 06:57 -0400, Frediano Ziglio wrote:
>>>>>
>>>>>
>>>>> When MainChannelClient was split to a separate file, the
>>>>> responsibility
>>>>> for incrementing this field was supposed to belong to the
>>>>> MainChannel
>>>>> function (main_channel_connect_semi_seamless()), but by
>>>>> mistake it
>>>>> was
>>>>> incremented both there and in the client function
>>>>> (main_channel_client_connect_semi_seamless()).
>>>>>
>>>>> The bug was introduced in
>>>>> a11b785f191d2381932f8c1bb6281539f2afe483
>>>>> ---
>>>>>  server/main-channel-client.c | 2 --
>>>>>  1 file changed, 2 deletions(-)
>>>>>
>>>>> diff --git a/server/main-channel-client.c b/server/main-
>>>>> channel-
>>>>> client.c
>>>>> index b30083f..0913028 100644
>>>>> --- a/server/main-channel-client.c
>>>>> +++ b/server/main-channel-client.c
>>>>> @@ -704,7 +704,6 @@ void
>>>>> main_channel_client_migrate(RedChannelClient *rcc)
>>>>>  gboolean
>>>>> main_channel_client_connect_semi_seamless(MainChannelClient
>>>>> *mcc)
>>>>>  {
>>>>>      RedChannelClient *rcc = RED_CHANNEL_CLIENT(mcc);
>>>>> -    MainChannel* main_channel =
>>>>> MAIN_CHANNEL(red_channel_client_get_channel(rcc));
>>>>>      if (red_channel_client_test_remote_cap(rcc,
>>>>>                                             SPICE_MAIN_CAP_SEM
>>>>> I_SEA
>>>>> MLESS_MIGRATE))
>>>>>                                             {
>>>>>          RedClient *client =
>>>>> red_channel_client_get_client(rcc);
>>>>> @@ -718,7 +717,6 @@ gboolean
>>>>> main_channel_client_connect_semi_seamless(MainChannelClient
>>>>> *mcc)
>>>>>              mcc->priv->mig_wait_connect = TRUE;
>>>>>          }
>>>>>          mcc->priv->mig_connect_ok = FALSE;
>>>>> -        main_channel->num_clients_mig_wait++;
>>>>>          return TRUE;
>>>>>      }
>>>>>      return FALSE;
>>>>
>>>> Acked-by: Frediano Ziglio <fziglio at redhat.com>
>>>>
>>>> I think we have a lack of testing, at least fast ones.
>>>
>>> Yes, definitely.
>>>
>>>> How did you discover this?
>>>
>>> Just code inspection. While splitting out the MainChannelPrivate
>>> stuff
>>> into a separate patch, I noticed that this hunk was included in
>>> the
>>> "Convert RedChannel Hierarchy to GObject" patch. And it looked odd
>>> to
>>> me, so I went to look at the history of this particular code.
>>>
>>>> Did you manage to reproduce the issue?
>>>
>>> No.
>>>
>>
>> I remember Uri can test migration locally but I never dig around
>> enough.
>> Uri, how to test it locally?
>
> Take a look at:
> https://cgit.freedesktop.org/spice/spice/tree/tests/migrate.py

That's a good test.
Seems like an n'th version of a test I wrote long ago :-)

Would be nice to add seamless migration support.

Frediano, to do it manually I basically run the same qemu-kvm
command in two terminals, one is the migration source and
the other migration destination. On a third terminal I run the client.
You have to give them different spice-port and destination
has to have incoming tcp:localhost:$migration-port
For each src/dst I add "monitor=stdio" such that it accepts
commands from stdin.
For seamless migration you want to add ",seamless-migration=on" to the 
command line and issue qemu monitor command on the src:
(qemu-src) client_migrate_info spice localhost $remote-spice-port

You can verify the client is connected to dst with (not needed)
(qemu-dst) info spice

and than migrate
(qemu-src) migrate [-d (*)] tcp:localhost:$migration-port

you can make it go faster/slower (*) with
(qemu-src) migrate_set_speed

and query the status (*) with
(qemu-src) info migrate

(*) You can issue those commands during migration if
     you ran migrate with "-d"

Upon successful completion quit the src as dst is now
the active instance of the VM.
(qemu-src) quit


Uri.