Arun Menon <arme...@redhat.com> writes:
> 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.
>
> Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> Signed-off-by: Arun Menon <arme...@redhat.com>
> ---
> migration/savevm.c | 82
> +++++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 60 insertions(+), 22 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index
> 7f79461844105bf672314c3325caee9cdb654c27..715cc35394cac5fe225ef88cf448714a02321bd7
> 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2546,32 +2546,35 @@ 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);
>
> /* Check validity before continue processing of cmds */
> - if (qemu_file_get_error(f)) {
> - return qemu_file_get_error(f);
> + ret = qemu_file_get_error(f);
> + if (ret) {
> + error_setg(errp, "Failed to load VM process command: %d", ret);
"Failed to process command: stream error: %d"
> + return ret;
> }
>
> 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;
> }
>
> trace_loadvm_process_command(mig_cmd_args[cmd].name, len);
>
> if (mig_cmd_args[cmd].len != -1 && mig_cmd_args[cmd].len != len) {
> - error_report("%s received with bad length - expecting %zu, got %d",
> - mig_cmd_args[cmd].name,
> - (size_t)mig_cmd_args[cmd].len, len);
> + error_setg(errp, "%s received with bad length - expecting %zu, got
> %d",
> + mig_cmd_args[cmd].name,
> + (size_t)mig_cmd_args[cmd].len, len);
> return -ERANGE;
> }
>
Where's MIG_CMD_OPEN_RETURN_PATH?
> @@ -2594,11 +2597,10 @@ static int loadvm_process_command(QEMUFile *f)
> * been created.
> */
> if (migrate_switchover_ack() && !mis->switchover_ack_pending_num) {
> - int ret = migrate_send_rp_switchover_ack(mis);
> + ret = migrate_send_rp_switchover_ack(mis);
> if (ret) {
> - error_report(
> - "Could not send switchover ack RP MSG, err %d (%s)", ret,
> - strerror(-ret));
> + error_setg_errno(errp, -ret,
> + "Could not send switchover ack RP MSG");
> return ret;
> }
> }
> @@ -2608,39 +2610,71 @@ static int loadvm_process_command(QEMUFile *f)
> tmp32 = qemu_get_be32(f);
> trace_loadvm_process_command_ping(tmp32);
> if (!mis->to_src_file) {
> - error_report("CMD_PING (0x%x) received with no return path",
> - tmp32);
> + error_setg(errp, "CMD_PING (0x%x) received with no return path",
> + tmp32);
> return -1;
> }
> migrate_send_rp_pong(mis, tmp32);
> break;
>
> case MIG_CMD_PACKAGED:
> - return loadvm_handle_cmd_packaged(mis);
> + ret = loadvm_handle_cmd_packaged(mis);
I missed a lot of the discussion in this series, but I assume there's a
good reason to not put the conversion of each command first in the
series, so there's no need for temporary code in this patch.
> + if (ret < 0) {
> + error_setg(errp, "Failed to load device state command: %d", ret);
> + }
> + 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 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 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 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 ret;
>
> case MIG_CMD_POSTCOPY_RESUME:
> return loadvm_postcopy_handle_resume(mis);
>
> 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 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 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 ret;
> }
>
> return 0;
> @@ -3051,6 +3085,7 @@ int qemu_loadvm_state_main(QEMUFile *f,
> MigrationIncomingState *mis)
> {
> uint8_t section_type;
> int ret = 0;
> + Error *local_err = NULL;
>
> retry:
> while (true) {
> @@ -3078,7 +3113,10 @@ retry:
> }
> break;
> case QEMU_VM_COMMAND:
> - ret = loadvm_process_command(f);
> + ret = loadvm_process_command(f, &local_err);
> + if (ret < 0) {
> + warn_report_err(local_err);
Again, some throwaway code here. Commit message could have made this
clear: "For now, report the error with warn_report until all callers
have been converted to pass errp".
> + }
> trace_qemu_loadvm_state_section_command(ret);
> if ((ret < 0) || (ret == LOADVM_QUIT)) {
> goto out;
