Re: [PATCH v5 07/23] migration: push Error **errp into loadvm_process_command()

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Jul 17, 2025 at 06:07:30AM +0530, Arun Menon wrote:
> This is an incremental step in converting vmstate loading
> code to report error via Error objects instead of directly
> printing it to console/monitor.
> It is ensured that loadvm_process_command() must report an error
> in errp, in case of failure.
> 
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/savevm.c | 89 ++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 67 insertions(+), 22 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 22d73999595384519c755c9416b74ba1263a8bb9..98711bb38a4548d4f168459f729f604a78716c25 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2540,12 +2540,13 @@ static int loadvm_postcopy_handle_switchover_start(void)
>   * LOADVM_QUIT All good, but exit the loop
>   * <0          Error
>   */
> -static int loadvm_process_command(QEMUFile *f)
> +static int loadvm_process_command(QEMUFile *f, Error **errp)
>  {
>      MigrationIncomingState *mis = migration_incoming_get_current();
>      uint16_t cmd;
>      uint16_t len;
>      uint32_t tmp32;
> +    int ret;
>  
>      cmd = qemu_get_be16(f);
>      len = qemu_get_be16(f);
> @@ -2556,16 +2557,16 @@ static int loadvm_process_command(QEMUFile *f)
>      }
>  
>      if (cmd >= MIG_CMD_MAX || cmd == MIG_CMD_INVALID) {
> -        error_report("MIG_CMD 0x%x unknown (len 0x%x)", cmd, len);
> +        error_setg(errp, "MIG_CMD 0x%x unknown (len 0x%x)", cmd, len);
>          return -EINVAL;
>      }
>  


> @@ -2578,7 +2579,7 @@ static int loadvm_process_command(QEMUFile *f)
>          }
>          mis->to_src_file = qemu_file_get_return_path(f);
>          if (!mis->to_src_file) {
> -            error_report("CMD_OPEN_RETURN_PATH failed");
> +            error_setg(errp, "CMD_OPEN_RETURN_PATH failed");

This is a wierd one, according to the docs qemu_file_get_return_path
can return NULL, but according to the impl it will always succeed.

Suggest creating an earlier patch that fixes the docs and removes
the check for failure here, not least because this existing error
message provides no useful information, and none will be available
since it can't fail.


>      case MIG_CMD_PACKAGED:
> -        return loadvm_handle_cmd_packaged(mis);
> +        ret = loadvm_handle_cmd_packaged(mis);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +            return -1;
> +        }
> +        return ret;
>  
>      case MIG_CMD_POSTCOPY_ADVISE:
> -        return loadvm_postcopy_handle_advise(mis, len);
> +        ret = loadvm_postcopy_handle_advise(mis, len);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +            return -1;
> +        }
> +        return ret;
>  
>      case MIG_CMD_POSTCOPY_LISTEN:
> -        return loadvm_postcopy_handle_listen(mis);
> +        ret = loadvm_postcopy_handle_listen(mis);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +            return -1;
> +        }
> +        return ret;
>  
>      case MIG_CMD_POSTCOPY_RUN:
> -        return loadvm_postcopy_handle_run(mis);
> +        ret = loadvm_postcopy_handle_run(mis);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +            return -1;
> +        }
> +        return ret;
>  
>      case MIG_CMD_POSTCOPY_RAM_DISCARD:
> -        return loadvm_postcopy_ram_handle_discard(mis, len);
> +        ret = loadvm_postcopy_ram_handle_discard(mis, len);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +            return -1;
> +        }
> +        return ret;
>  
>      case MIG_CMD_POSTCOPY_RESUME:
> -        return loadvm_postcopy_handle_resume(mis);
> +        ret = loadvm_postcopy_handle_resume(mis);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +            return -1;
> +        }
> +        return ret;
>  
>      case MIG_CMD_RECV_BITMAP:
> -        return loadvm_handle_recv_bitmap(mis, len);
> +        ret = loadvm_handle_recv_bitmap(mis, len);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +            return -1;
> +        }
> +        return ret;
>  
>      case MIG_CMD_ENABLE_COLO:
> -        return loadvm_process_enable_colo(mis);
> +        ret = loadvm_process_enable_colo(mis);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +            return -1;
> +        }
> +        return ret;
>  
>      case MIG_CMD_SWITCHOVER_START:
> -        return loadvm_postcopy_handle_switchover_start();
> +        ret = loadvm_postcopy_handle_switchover_start();
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +            return -1;
> +        }
> +        return ret;
>      }

Can you ensure that each of these have unique error message strings,
as if they ever appear, it will be important to distinguish which of
these nine commands actually failed.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|