From 540d91b40c390faa565e613a85bc0950ca7e0775 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 13 May 2024 16:16:58 +0200 Subject: [PATCH 1/9] block: Improve error message when external snapshot can't flush MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit external_snapshot_action() reports bdrv_flush() failure to its caller as An IO error has occurred The errno code returned by bdrv_flush() is lost. Improve this to Write to node '' failed: Signed-off-by: Markus Armbruster Message-ID: <20240513141703.549874-2-armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé --- blockdev.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/blockdev.c b/blockdev.c index 08eccc9052..528db3452f 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1406,8 +1406,10 @@ static void external_snapshot_action(TransactionAction *action, } if (!bdrv_is_read_only(state->old_bs)) { - if (bdrv_flush(state->old_bs)) { - error_setg(errp, QERR_IO_ERROR); + ret = bdrv_flush(state->old_bs); + if (ret < 0) { + error_setg_errno(errp, -ret, "Write to node '%s' failed", + bdrv_get_device_or_node_name(state->old_bs)); return; } } From 21c06f57808c7236cca68ccc7d8df845c22cd998 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 13 May 2024 16:16:59 +0200 Subject: [PATCH 2/9] dump/win_dump: Improve error messages on write error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit create_win_dump() and write_run report qemu_write_full() failure to their callers as An IO error has occurred The errno set by qemu_write_full() is lost. Improve this to win-dump: failed to write header: and win-dump: failed to save memory: This matches how dump.c reports similar errors. Signed-off-by: Markus Armbruster Message-ID: <20240513141703.549874-3-armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé --- dump/win_dump.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/dump/win_dump.c b/dump/win_dump.c index b7bfaff379..0e4fe692ce 100644 --- a/dump/win_dump.c +++ b/dump/win_dump.c @@ -12,7 +12,6 @@ #include "sysemu/dump.h" #include "qapi/error.h" #include "qemu/error-report.h" -#include "qapi/qmp/qerror.h" #include "exec/cpu-defs.h" #include "hw/core/cpu.h" #include "qemu/win_dump_defs.h" @@ -52,6 +51,7 @@ static size_t write_run(uint64_t base_page, uint64_t page_count, uint64_t addr = base_page << TARGET_PAGE_BITS; uint64_t size = page_count << TARGET_PAGE_BITS; uint64_t len, l; + int eno; size_t total = 0; while (size) { @@ -65,9 +65,10 @@ static size_t write_run(uint64_t base_page, uint64_t page_count, } l = qemu_write_full(fd, buf, len); + eno = errno; cpu_physical_memory_unmap(buf, addr, false, len); if (l != len) { - error_setg(errp, QERR_IO_ERROR); + error_setg_errno(errp, eno, "win-dump: failed to save memory"); return 0; } @@ -459,7 +460,7 @@ void create_win_dump(DumpState *s, Error **errp) s->written_size = qemu_write_full(s->fd, h, hdr_size); if (s->written_size != hdr_size) { - error_setg(errp, QERR_IO_ERROR); + error_setg_errno(errp, errno, "win-dump: failed to write header"); goto out_restore; } From 29ad187c1c89fcf82d4e73e90bec9722d548efed Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 13 May 2024 16:17:00 +0200 Subject: [PATCH 3/9] block/vmdk: Improve error messages on extent write error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vmdk_init_extent() reports blk_co_pwrite() failure to its caller as An IO error has occurred The errno code returned by blk_co_pwrite() is lost. Improve this to failed to write VMDK : Signed-off-by: Markus Armbruster Message-ID: <20240513141703.549874-4-armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé --- block/vmdk.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index 3b82979fdf..78f6433607 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -28,7 +28,6 @@ #include "block/block_int.h" #include "sysemu/block-backend.h" #include "qapi/qmp/qdict.h" -#include "qapi/qmp/qerror.h" #include "qemu/error-report.h" #include "qemu/module.h" #include "qemu/option.h" @@ -2278,12 +2277,12 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress, /* write all the data */ ret = blk_co_pwrite(blk, 0, sizeof(magic), &magic, 0); if (ret < 0) { - error_setg(errp, QERR_IO_ERROR); + error_setg_errno(errp, -ret, "failed to write VMDK magic"); goto exit; } ret = blk_co_pwrite(blk, sizeof(magic), sizeof(header), &header, 0); if (ret < 0) { - error_setg(errp, QERR_IO_ERROR); + error_setg_errno(errp, -ret, "failed to write VMDK header"); goto exit; } @@ -2303,7 +2302,7 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress, ret = blk_co_pwrite(blk, le64_to_cpu(header.rgd_offset) * BDRV_SECTOR_SIZE, gd_buf_size, gd_buf, 0); if (ret < 0) { - error_setg(errp, QERR_IO_ERROR); + error_setg_errno(errp, -ret, "failed to write VMDK grain directory"); goto exit; } @@ -2315,7 +2314,8 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress, ret = blk_co_pwrite(blk, le64_to_cpu(header.gd_offset) * BDRV_SECTOR_SIZE, gd_buf_size, gd_buf, 0); if (ret < 0) { - error_setg(errp, QERR_IO_ERROR); + error_setg_errno(errp, -ret, + "failed to write VMDK backup grain directory"); } ret = 0; From c59fb13be7fd937c087ed103abad4c2d9d2bcfab Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 13 May 2024 16:17:01 +0200 Subject: [PATCH 4/9] cpus: Improve error messages on memsave, pmemsave write error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit qmp_memsave() and qmp_pmemsave() report fwrite() error as An IO error has occurred Improve this to writing memory to '' failed Signed-off-by: Markus Armbruster Message-ID: <20240513141703.549874-5-armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé --- system/cpus.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/system/cpus.c b/system/cpus.c index 68d161d96b..f8fa78f33d 100644 --- a/system/cpus.c +++ b/system/cpus.c @@ -813,7 +813,8 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename, goto exit; } if (fwrite(buf, 1, l, f) != l) { - error_setg(errp, QERR_IO_ERROR); + error_setg(errp, "writing memory to '%s' failed", + filename); goto exit; } addr += l; @@ -843,7 +844,8 @@ void qmp_pmemsave(int64_t addr, int64_t size, const char *filename, l = size; cpu_physical_memory_read(addr, buf, l); if (fwrite(buf, 1, l, f) != l) { - error_setg(errp, QERR_IO_ERROR); + error_setg(errp, "writing memory to '%s' failed", + filename); goto exit; } addr += l; From fdac62dbd3de7e65c5a79798097894a29125c60e Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 13 May 2024 16:17:02 +0200 Subject: [PATCH 5/9] migration: Rephrase message on failure to save / load Xen device state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Functions that use an Error **errp parameter to return errors should not also report them to the user, because reporting is the caller's job. When the caller does, the error is reported twice. When it doesn't (because it recovered from the error), there is no error to report, i.e. the report is bogus. qmp_xen_save_devices_state() and qmp_xen_load_devices_state() violate this principle: they call qemu_save_device_state() and qemu_loadvm_state(), which call error_report_err(). I wish I could clean this up now, but migration's error reporting is too complicated (confused?) for me to mess with it. Instead, I'm merely improving the error reported by qmp_xen_load_devices_state() and qmp_xen_load_devices_state() to the QMP core from An IO error has occurred to saving Xen device state failed and loading Xen device state failed respectively. Signed-off-by: Markus Armbruster Message-ID: <20240513141703.549874-6-armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé Acked-by: Fabiano Rosas Acked-by: Peter Xu --- migration/savevm.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/migration/savevm.c b/migration/savevm.c index 6c789bd54b..c621f2359b 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -45,7 +45,6 @@ #include "qapi/qapi-commands-migration.h" #include "qapi/clone-visitor.h" #include "qapi/qapi-builtin-visit.h" -#include "qapi/qmp/qerror.h" #include "qemu/error-report.h" #include "sysemu/cpus.h" #include "exec/memory.h" @@ -3203,7 +3202,7 @@ void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live, object_unref(OBJECT(ioc)); ret = qemu_save_device_state(f); if (ret < 0 || qemu_fclose(f) < 0) { - error_setg(errp, QERR_IO_ERROR); + error_setg(errp, "saving Xen device state failed"); } else { /* libxl calls the QMP command "stop" before calling * "xen-save-devices-state" and in case of migration failure, libxl @@ -3252,7 +3251,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp) ret = qemu_loadvm_state(f); qemu_fclose(f); if (ret < 0) { - error_setg(errp, QERR_IO_ERROR); + error_setg(errp, "loading Xen device state failed"); } migration_incoming_state_destroy(); } From 5b957bf6d378cfa0c196b2be119449e086647612 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 13 May 2024 16:17:03 +0200 Subject: [PATCH 6/9] qerror: QERR_IO_ERROR is no longer used, drop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Markus Armbruster Message-ID: <20240513141703.549874-7-armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé --- include/qapi/qmp/qerror.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h index 00b18e9082..bc9116f76a 100644 --- a/include/qapi/qmp/qerror.h +++ b/include/qapi/qmp/qerror.h @@ -20,9 +20,6 @@ #define QERR_INVALID_PARAMETER_VALUE \ "Parameter '%s' expects %s" -#define QERR_IO_ERROR \ - "An IO error has occurred" - #define QERR_MISSING_PARAMETER \ "Parameter '%s' is missing" From ecfc9890c41541fb08ce07a6ff26e7bd14f74967 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 14 May 2024 12:58:26 +0200 Subject: [PATCH 7/9] qga-win32: Improve guest-set-user-password, guest-file-open errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When guest-set-user-password's argument @password can't be converted from UTF-8 to UTF-16, we report something like Guest agent command failed, error was 'Invalid sequence in conversion input' Improve this to can't convert 'password' to UTF-16: Invalid sequence in conversion input Likewise for argument @username, and guest-file-open argument @path, even though I'm not sure you can actually get invalid input past the QMP core there. Signed-off-by: Markus Armbruster Message-ID: <20240514105829.729342-2-armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Konstantin Kostiuk --- qga/commands-win32.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 6fee0e1e6f..ed31077457 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -217,6 +217,9 @@ int64_t qmp_guest_file_open(const char *path, const char *mode, Error **errp) w_path = g_utf8_to_utf16(path, -1, NULL, NULL, &gerr); if (!w_path) { + error_setg(errp, "can't convert 'path' to UTF-16: %s", + gerr->message); + g_error_free(gerr); goto done; } @@ -244,10 +247,6 @@ int64_t qmp_guest_file_open(const char *path, const char *mode, Error **errp) slog("guest-file-open, handle: % " PRId64, fd); done: - if (gerr) { - error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message); - g_error_free(gerr); - } g_free(w_path); return fd; } @@ -1946,11 +1945,17 @@ void qmp_guest_set_user_password(const char *username, user = g_utf8_to_utf16(username, -1, NULL, NULL, &gerr); if (!user) { + error_setg(errp, "can't convert 'username' to UTF-16: %s", + gerr->message); + g_error_free(gerr); goto done; } wpass = g_utf8_to_utf16(rawpasswddata, -1, NULL, NULL, &gerr); if (!wpass) { + error_setg(errp, "can't convert 'password' to UTF-16: %s", + gerr->message); + g_error_free(gerr); goto done; } @@ -1966,10 +1971,6 @@ void qmp_guest_set_user_password(const char *username, } done: - if (gerr) { - error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message); - g_error_free(gerr); - } g_free(user); g_free(wpass); g_free(rawpasswddata); From cec07c79a41827689f9b9ea7be47fd1ae17a1242 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 14 May 2024 12:58:27 +0200 Subject: [PATCH 8/9] qga: Shorten several error messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some, but not all error messages are of the form Guest agent command failed, error was '' For instance, command guest-exec can fail with an error message like Guest agent command failed, error was 'Failed to execute child process “/bin/invalid-cmd42” (No such file or directory)' Shorten this to just just the actual error message. The guest-exec example becomes Failed to execute child process “/bin/invalid-cmd42” (No such file or directory) Signed-off-by: Markus Armbruster Message-ID: <20240514105829.729342-3-armbru@redhat.com> [Superfluous #include "qapi/qmp/qerror.h" deleted] Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Konstantin Kostiuk --- qga/commands-win32.c | 24 ++++++++---------------- qga/commands.c | 6 ++---- 2 files changed, 10 insertions(+), 20 deletions(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index ed31077457..0d1b836e87 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -278,8 +278,7 @@ static void acquire_privilege(const char *name, Error **errp) TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY, &token)) { if (!LookupPrivilegeValue(NULL, name, &priv.Privileges[0].Luid)) { - error_setg(errp, QERR_QGA_COMMAND_FAILED, - "no luid for requested privilege"); + error_setg(errp, "no luid for requested privilege"); goto out; } @@ -287,14 +286,12 @@ static void acquire_privilege(const char *name, Error **errp) priv.Privileges[0].Attributes = SE_PRIVILEGE_ENABLED; if (!AdjustTokenPrivileges(token, FALSE, &priv, 0, NULL, 0)) { - error_setg(errp, QERR_QGA_COMMAND_FAILED, - "unable to acquire requested privilege"); + error_setg(errp, "unable to acquire requested privilege"); goto out; } } else { - error_setg(errp, QERR_QGA_COMMAND_FAILED, - "failed to open privilege token"); + error_setg(errp, "failed to open privilege token"); } out: @@ -308,8 +305,7 @@ static void execute_async(DWORD WINAPI (*func)(LPVOID), LPVOID opaque, { HANDLE thread = CreateThread(NULL, 0, func, opaque, 0, NULL); if (!thread) { - error_setg(errp, QERR_QGA_COMMAND_FAILED, - "failed to dispatch asynchronous command"); + error_setg(errp, "failed to dispatch asynchronous command"); } } @@ -1418,22 +1414,19 @@ static void check_suspend_mode(GuestSuspendMode mode, Error **errp) ZeroMemory(&sys_pwr_caps, sizeof(sys_pwr_caps)); if (!GetPwrCapabilities(&sys_pwr_caps)) { - error_setg(errp, QERR_QGA_COMMAND_FAILED, - "failed to determine guest suspend capabilities"); + error_setg(errp, "failed to determine guest suspend capabilities"); return; } switch (mode) { case GUEST_SUSPEND_MODE_DISK: if (!sys_pwr_caps.SystemS4) { - error_setg(errp, QERR_QGA_COMMAND_FAILED, - "suspend-to-disk not supported by OS"); + error_setg(errp, "suspend-to-disk not supported by OS"); } break; case GUEST_SUSPEND_MODE_RAM: if (!sys_pwr_caps.SystemS3) { - error_setg(errp, QERR_QGA_COMMAND_FAILED, - "suspend-to-ram not supported by OS"); + error_setg(errp, "suspend-to-ram not supported by OS"); } break; default: @@ -2175,8 +2168,7 @@ static void ga_get_win_version(RTL_OSVERSIONINFOEXW *info, Error **errp) HMODULE module = GetModuleHandle("ntdll"); PVOID fun = GetProcAddress(module, "RtlGetVersion"); if (fun == NULL) { - error_setg(errp, QERR_QGA_COMMAND_FAILED, - "Failed to get address of RtlGetVersion"); + error_setg(errp, "Failed to get address of RtlGetVersion"); return; } diff --git a/qga/commands.c b/qga/commands.c index 88c1c99fe5..5a5fad31f8 100644 --- a/qga/commands.c +++ b/qga/commands.c @@ -15,7 +15,6 @@ #include "guest-agent-core.h" #include "qga-qapi-commands.h" #include "qapi/error.h" -#include "qapi/qmp/qerror.h" #include "qemu/base64.h" #include "qemu/cutils.h" #include "commands-common.h" @@ -475,7 +474,7 @@ GuestExec *qmp_guest_exec(const char *path, guest_exec_task_setup, &has_merge, &pid, input_data ? &in_fd : NULL, has_output ? &out_fd : NULL, has_output ? &err_fd : NULL, &gerr); if (!ret) { - error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message); + error_setg(errp, "%s", gerr->message); g_error_free(gerr); goto done; } @@ -586,8 +585,7 @@ GuestTimezone *qmp_guest_get_timezone(Error **errp) info = g_new0(GuestTimezone, 1); tz = g_time_zone_new_local(); if (tz == NULL) { - error_setg(errp, QERR_QGA_COMMAND_FAILED, - "Couldn't retrieve local timezone"); + error_setg(errp, "Couldn't retrieve local timezone"); goto error; } From 2020337239b054447d0756ea75e970b35f7a541b Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 14 May 2024 12:58:28 +0200 Subject: [PATCH 9/9] qerror: QERR_QGA_COMMAND_FAILED is no longer used, drop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Markus Armbruster Message-ID: <20240514105829.729342-4-armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Konstantin Kostiuk --- include/qapi/qmp/qerror.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h index bc9116f76a..38e89762b3 100644 --- a/include/qapi/qmp/qerror.h +++ b/include/qapi/qmp/qerror.h @@ -26,9 +26,6 @@ #define QERR_PROPERTY_VALUE_OUT_OF_RANGE \ "Property %s.%s doesn't take value %" PRId64 " (minimum: %" PRId64 ", maximum: %" PRId64 ")" -#define QERR_QGA_COMMAND_FAILED \ - "Guest agent command failed, error was '%s'" - #define QERR_UNSUPPORTED \ "this feature or command is not currently supported"