[Spice-devel] [PATCH] migration: Don't assert() if MIGRATE_DATA comes before attaching the agent

[Uri Lublin]

On 02/24/2014 07:44 PM, Christophe Fergeau wrote:
> During seamless migration, after switching host, if a client was connected
> during the migration, it will have data to send back to the new
> qemu/spice-server instance. This is handled through MIGRATE_DATA messages.
> SPICE char devices use such MIGRATE_DATA messages to restore their state.
>
> However, the MIGRATE_DATA message can arrive any time after the new qemu
> instance has started, this can happen before or after the SPICE char
> devices have been created. In order to handle this, if the migrate data
> arrives early, it's stored in reds->agent_state.mig_data, and
> attach_to_red_agent() will restore the agent state as appropriate.
>
> Unfortunately this does not work as expected as expected. If
> attach_to_red_agent() is called before the MIGRATE_DATA message reaches the
> server, all goes well, but if MIGRATE_DATA reaches the server before
> attach_to_red_agent() gets called, then some assert() get triggered in
> spice_char_device_state_restore():
>
> ((null):32507): Spice-ERROR **: char_device.c:937:spice_char_device_state_restore: assertion `dev->num_clients == 1 && dev->wait_for_migrate_data' failed
> Thread 3 (Thread 0x7f406b543700 (LWP 32543)):
> Thread 2 (Thread 0x7f40697ff700 (LWP 32586)):
> Thread 1 (Thread 0x7f4079b45a40 (LWP 32507)):
>
> What happens is that dev->wait_for_migrate_data is unconditionally cleared when
> completing handling of the MIGRATE_DATA message, so it's FALSE when
> spice_char_device_state_restore() is called. Moreover, dev->num_clients is
> also 0 as this is only increased by spice_char_device_start() which
> spice_server_char_device_add_interface() calls after
> attach_to_red_agent()/spice_char_device_state_restore() have been called.
>
> This commit changes the logic in spice_server_char_device_add_interface(),
> and when there is migrate data pending in reds->agent_state.mig_data, we
> only attempt to restore it after successfully initializing/starting the
> needed char device.
>
> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1035184
> ---
>   server/reds.c | 26 +++++++++++++++-----------
>   1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/server/reds.c b/server/reds.c
> index 1f02553..217c74e 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2856,16 +2856,7 @@ static SpiceCharDeviceState *attach_to_red_agent(SpiceCharDeviceInstance *sin)
>       state->read_filter.discard_all = FALSE;
>       reds->agent_state.plug_generation++;
>   
> -    if (reds->agent_state.mig_data) {
> -        spice_assert(reds->agent_state.plug_generation == 1);
> -        reds_agent_state_restore(reds->agent_state.mig_data);
> -        free(reds->agent_state.mig_data);
> -        reds->agent_state.mig_data = NULL;
> -    } else if (!red_channel_waits_for_migrate_data(&reds->main_channel->base)) {
> -        /* we will assoicate the client with the char device, upon reds_on_main_agent_start,
> -         * in response to MSGC_AGENT_START */
> -        main_channel_push_agent_connected(reds->main_channel);
> -    } else {
> +    if (red_channel_waits_for_migrate_data(&reds->main_channel->base) || reds->agent_state.mig_data) {
>          spice_debug("waiting for migration data");
>           if (!spice_char_device_client_exists(reds->agent_state.base, reds_get_client())) {
>               int client_added;
> @@ -2882,9 +2873,13 @@ static SpiceCharDeviceState *attach_to_red_agent(SpiceCharDeviceInstance *sin)
>                   spice_warning("failed to add client to agent");
>                   reds_disconnect();
>               }
> -
>           }
> +    } else {
> +        /* we will associate the client with the char device, upon reds_on_main_agent_start,
> +         * in response to MSGC_AGENT_START */
> +        main_channel_push_agent_connected(reds->main_channel);
>       }
> +
>       return state->base;
>   }
>   
> @@ -2990,6 +2985,15 @@ static int spice_server_char_device_add_interface(SpiceServer *s,
>           spice_warning("failed to create device state for %s", char_device->subtype);
>           return -1;
>       }
> +
> +    if (strcmp(char_device->subtype, SUBTYPE_VDAGENT) == 0) {
> +        if (reds->agent_state.mig_data) {
> +               spice_assert(reds->agent_state.plug_generation == 1);
> +               reds_agent_state_restore(reds->agent_state.mig_data);
> +               free(reds->agent_state.mig_data);
> +               reds->agent_state.mig_data = NULL;
> +        }
> +    }
>       return 0;
>   }

Hi Christophe,

I'm concerned that reds_agent_state_restore() is called after 
spice_char_device_start()

Also I'm not sure how this patch solves the race.
This patch moves the call to reds_agent_state_restore() from 
attach_to_red_agent()
into spice_server_char_device_add_interface(). But attach_to_red_agent 
is only called
from spice_server_char_devices_add_interface.

Thanks,
     Uri.