On 2025/08/01 22:27, Arun Menon wrote:
> Hi Akihiko,
>
> Thanks for the review.
>
> On Fri, Aug 01, 2025 at 04:12:34PM +0900, Akihiko Odaki wrote:
>> On 2025/07/31 22:20, 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 qemu_loadvm_section_start_full() must report an error
>>> in errp, in case of failure.
>>>
>>> Signed-off-by: Arun Menon <armenon@redhat.com>
>>> ---
>>> migration/savevm.c | 36 ++++++++++++++++++++----------------
>>> 1 file changed, 20 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>> index 736410be867a29efa24d749528c9bc203a3e8131..59751677c1bb7c893b4ba61cbfe1f55ade6ad598 100644
>>> --- a/migration/savevm.c
>>> +++ b/migration/savevm.c
>>> @@ -2690,8 +2690,9 @@ static bool check_section_footer(QEMUFile *f, SaveStateEntry *se)
>>> }
>>> static int
>>> -qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
>>> +qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp)
>>> {
>>> + ERRP_GUARD();
>>> bool trace_downtime = (type == QEMU_VM_SECTION_FULL);
>>> uint32_t instance_id, version_id, section_id;
>>> int64_t start_ts, end_ts;
>>> @@ -2702,8 +2703,8 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
>>> /* Read section start */
>>> section_id = qemu_get_be32(f);
>>> if (!qemu_get_counted_string(f, idstr)) {
>>> - error_report("Unable to read ID string for section %u",
>>> - section_id);
>>> + error_setg(errp, "Unable to read ID string for section %u",
>>> + section_id);
>>> return -EINVAL;
>>> }
>>> instance_id = qemu_get_be32(f);
>>> @@ -2711,8 +2712,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
>>> ret = qemu_file_get_error(f);
>>> if (ret) {
>>> - error_report("%s: Failed to read instance/version ID: %d",
>>> - __func__, ret);
>>> + error_setg(errp, "Failed to read instance/version ID: %d", ret);
>>> return ret;
>>> }
>>> @@ -2721,17 +2721,17 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
>>> /* Find savevm section */
>>> se = find_se(idstr, instance_id);
>>> if (se == NULL) {
>>> - error_report("Unknown savevm section or instance '%s' %"PRIu32". "
>>> - "Make sure that your current VM setup matches your "
>>> - "saved VM setup, including any hotplugged devices",
>>> - idstr, instance_id);
>>> + error_setg(errp, "Unknown savevm section or instance '%s' %"PRIu32". "
>>> + "Make sure that your current VM setup matches your "
>>> + "saved VM setup, including any hotplugged devices",
>>> + idstr, instance_id);
>>> return -EINVAL;
>>> }
>>> /* Validate version */
>>> if (version_id > se->version_id) {
>>> - error_report("savevm: unsupported version %d for '%s' v%d",
>>> - version_id, idstr, se->version_id);
>>> + error_setg(errp, "savevm: unsupported version %d for '%s' v%d",
>>> + version_id, idstr, se->version_id);
>>> return -EINVAL;
>>> }
>>> se->load_version_id = version_id;
>>> @@ -2739,7 +2739,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
>>> /* Validate if it is a device's state */
>>> if (xen_enabled() && se->is_ram) {
>>> - error_report("loadvm: %s RAM loading not allowed on Xen", idstr);
>>> + error_setg(errp, "loadvm: %s RAM loading not allowed on Xen", idstr);
>>> return -EINVAL;
>>> }
>>> @@ -2747,10 +2747,11 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
>>> start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
>>> }
>>> - ret = vmstate_load(f, se, NULL);
>>> + ret = vmstate_load(f, se, errp);
>>> if (ret < 0) {
>>> - error_report("error while loading state for instance 0x%"PRIx32" of"
>>> - " device '%s'", instance_id, idstr);
>>> + error_prepend(errp,
>>> + "error while loading state for instance 0x%"PRIx32" of"
>>> + " device '%s': ", instance_id, idstr);
>>> return ret;
>>> }
>>> @@ -2761,6 +2762,9 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
>>> }
>>> if (!check_section_footer(f, se)) {
>>> + error_setg(errp, "Reading footer section of instance "
>>> + "0x%"PRIx32" of device '%s' for version_id:'%d' failed",
>>> + instance_id, idstr, version_id);
>>> return -EINVAL;
>>> }
>>> @@ -3063,7 +3067,7 @@ retry:
>>> switch (section_type) {
>>> case QEMU_VM_SECTION_START:
>>> case QEMU_VM_SECTION_FULL:
>>> - ret = qemu_loadvm_section_start_full(f, section_type);
>>> + ret = qemu_loadvm_section_start_full(f, section_type, NULL);
>>
>> The converted error_report() calls are temporarily dismissed. This can be
>> fixed by moving "[PATCH v8 19/27] migration: push Error **errp into
>> qemu_loadvm_state_main()" and changes for its caller earlier than this.
>>
>> qemu_loadvm_state_main() can have some code to fill errp until all the
>> functions its calls get converted. It will not make the patch bigger thanks
>> to the unified error handling path with "goto out" and the removal of code
>> changes to replace NULL with errp.
>
> I see your point.
> There is a cyclic dependency between few functions.
> For example, qemu_loadvm_state_main() calls -> loadvm_process_command()
> calls -> loadvm_handle_cmd_packaged()
> calls -> qemu_loadvm_state_main()
> That is why I passed NULL temporarily and then change that in the subsequent
> patches. However, I see that this will cause problems for reviewing and bisection.
> I can introduce a local_err in the caller, and when errp is available, I can pass that.
> That way I will be reporting local_err after it returns. That way the individual
> patches will report the error.
It is not necessary to introduce "local_err"; ERRP_GUARD() is added in
"[PATCH v8 19/27] migration: push Error **errp into
qemu_loadvm_state_main()" so you can check if *errp == NULL. This
hopefully break the cycle dependency without making patches bigger.
Regards,
Akihiko Odaki