[PATCH] migration/colo.c: Add missed return in error handling

[PATCH] migration/colo.c: Add missed return in error handling

[Rao Lei]

When doing failover and checkpoint, some returns are missed in error
handling. Let's add it.

Signed-off-by: Lei Rao <lei.rao@intel.com>
---
 migration/colo.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index 5f7071b3cd..014d3cba01 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -94,12 +94,15 @@ static void secondary_vm_do_failover(void)
     if (local_err) {
         error_report_err(local_err);
         local_err = NULL;
+        return;
     }
 
     /* Notify all filters of all NIC to do checkpoint */
     colo_notify_filters_event(COLO_EVENT_FAILOVER, &local_err);
     if (local_err) {
         error_report_err(local_err);
+        local_err = NULL;
+        return;
     }
 
     if (!autostart) {
@@ -178,6 +181,7 @@ static void primary_vm_do_failover(void)
     if (local_err) {
         error_report_err(local_err);
         local_err = NULL;
+        return;
     }
 
     /* Notify COLO thread that failover work is finished */
@@ -507,12 +511,11 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
         goto out;
     }
 
-    ret = 0;
-
     qemu_mutex_lock_iothread();
     vm_start();
     qemu_mutex_unlock_iothread();
     trace_colo_vm_state_change("stop", "run");
+    return 0;
 
 out:
     if (local_err) {
-- 
2.32.0

RE: [PATCH] migration/colo.c: Add missed return in error handling

[Zhang, Chen]

> -----Original Message-----
> From: Rao, Lei <lei.rao@intel.com>
> Sent: Tuesday, December 28, 2021 3:35 PM
> To: Zhang, Chen <chen.zhang@intel.com>; zhanghailiang@xfusion.com;
> quintela@redhat.com; dgilbert@redhat.com
> Cc: qemu-devel@nongnu.org; Rao, Lei <lei.rao@intel.com>
> Subject: [PATCH] migration/colo.c: Add missed return in error handling
> 
> When doing failover and checkpoint, some returns are missed in error
> handling. Let's add it.
> 
> Signed-off-by: Lei Rao <lei.rao@intel.com>

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Chen

> ---
>  migration/colo.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c index 5f7071b3cd..014d3cba01
> 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -94,12 +94,15 @@ static void secondary_vm_do_failover(void)
>      if (local_err) {
>          error_report_err(local_err);
>          local_err = NULL;
> +        return;
>      }
> 
>      /* Notify all filters of all NIC to do checkpoint */
>      colo_notify_filters_event(COLO_EVENT_FAILOVER, &local_err);
>      if (local_err) {
>          error_report_err(local_err);
> +        local_err = NULL;
> +        return;
>      }
> 
>      if (!autostart) {
> @@ -178,6 +181,7 @@ static void primary_vm_do_failover(void)
>      if (local_err) {
>          error_report_err(local_err);
>          local_err = NULL;
> +        return;
>      }
> 
>      /* Notify COLO thread that failover work is finished */ @@ -507,12 +511,11
> @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>          goto out;
>      }
> 
> -    ret = 0;
> -
>      qemu_mutex_lock_iothread();
>      vm_start();
>      qemu_mutex_unlock_iothread();
>      trace_colo_vm_state_change("stop", "run");
> +    return 0;
> 
>  out:
>      if (local_err) {
> --
> 2.32.0

Re: [PATCH] migration/colo.c: Add missed return in error handling

[Juan Quintela]

Rao Lei <lei.rao@intel.com> wrote:
> When doing failover and checkpoint, some returns are missed in error
> handling. Let's add it.
>
> Signed-off-by: Lei Rao <lei.rao@intel.com>
> ---
>  migration/colo.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/migration/colo.c b/migration/colo.c
> index 5f7071b3cd..014d3cba01 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -94,12 +94,15 @@ static void secondary_vm_do_failover(void)
>      if (local_err) {
>          error_report_err(local_err);
>          local_err = NULL;
> +        return;

Assign a local variable before a return is a NOP, so remove the assignmenent?

>      }
>  
>      /* Notify all filters of all NIC to do checkpoint */
>      colo_notify_filters_event(COLO_EVENT_FAILOVER, &local_err);
>      if (local_err) {
>          error_report_err(local_err);
> +        local_err = NULL;
> +        return;

Same here.

>      }
>  
>      if (!autostart) {
> @@ -178,6 +181,7 @@ static void primary_vm_do_failover(void)
>      if (local_err) {
>          error_report_err(local_err);
>          local_err = NULL;
> +        return;

And here.

>      }
>  
>      /* Notify COLO thread that failover work is finished */
> @@ -507,12 +511,11 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>          goto out;
>      }
>  
> -    ret = 0;
> -
>      qemu_mutex_lock_iothread();
>      vm_start();
>      qemu_mutex_unlock_iothread();
>      trace_colo_vm_state_change("stop", "run");
> +    return 0;
>  
>  out:
>      if (local_err) {

This is really a NOP, but it is one line less, so I will not complain.

But ther, it is better to just rename the label from "out" to "error" or
something like that.

Later, Juan.