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

[Christophe Fergeau]

Ping?

----- Mail original -----
> 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 (which will ensure that dev->num_clients is not 0).
> 
> It also changes attach_to_red_agent() to handle the case when
> reds->agent_state.mig_data is non-NULL to be the same as the case when
> red_channel_waits_for_migrate_data() is TRUE. This ensures that
> spice_char_device_client_add() gets called and that 'wait_for_data' is set
> in the added device.
> 
> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1035184
> ---
> Changes since v1:
> - Move block calling reds_agent_state_restore() before calling
>   spice_char_device_start() as this should be safer. This also
>   groups all SUBTYPE_xxx special-casing together. I've tested that
>   things are still working as expected after this move.
> 
> 
>  server/reds.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index 0390602..bd4fea1 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2863,16 +2863,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;
> @@ -2889,9 +2880,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;
>  }
>  
> @@ -2987,6 +2982,13 @@ static int
> spice_server_char_device_add_interface(SpiceServer *s,
>          } else {
>              dev_state = spicevmc_device_connect(char_device,
>              SPICE_CHANNEL_PORT);
>          }
> +    } else 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;
> +        }
>      }
>  
>      if (dev_state) {
> @@ -3001,6 +3003,7 @@ static int
> spice_server_char_device_add_interface(SpiceServer *s,
>          spice_warning("failed to create device state for %s",
>          char_device->subtype);
>          return -1;
>      }
> +
>      return 0;
>  }
>  
> --
> 1.9.0
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>