RE: [PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling

[Zhang, Chen]

> -----Original Message-----
> From: Markus Armbruster <address@hidden>
> Sent: Monday, April 20, 2020 4:33 PM
> To: address@hidden
> Cc: Zhang, Chen <address@hidden>; zhanghailiang
> <address@hidden>
> Subject: [PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint()
> error handling
> 
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the latter kind
> twice without clearing it in between is wrong: if the first call sets an 
> error, it
> no longer points to NULL for the second call.
> 
> qmp_xen_colo_do_checkpoint() passes @errp first to
> replication_do_checkpoint_all(), and then to colo_notify_filters_event().  If
> both fail, this will trip the assertion in error_setv().
> 
> Similar code in secondary_vm_do_failover() calls
> colo_notify_filters_event() only after replication_do_checkpoint_all()
> succeeded.  Do the same here.
> 
> Fixes: 0e8818f023616677416840d6ddc880db8de3c967
> Cc: Zhang Chen <address@hidden>
> Cc: zhanghailiang <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>

Looks good for me.

Reviewed-by: Zhang Chen <address@hidden>

Thanks
Zhang Chen

> ---
>  migration/colo.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c index a54ac84f41..1b3493729b
> 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -263,7 +263,13 @@ ReplicationStatus
> *qmp_query_xen_replication_status(Error **errp)
> 
>  void qmp_xen_colo_do_checkpoint(Error **errp)  {
> -    replication_do_checkpoint_all(errp);
> +    Error *err = NULL;
> +
> +    replication_do_checkpoint_all(&err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
>      /* Notify all filters of all NIC to do checkpoint */
>      colo_notify_filters_event(COLO_EVENT_CHECKPOINT, errp);  }
> --
> 2.21.1