This way we use the "normal" way of printing errors for hmp commands.
--
Paolo suggestion
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
hmp.c | 9 +++++++--
include/sysemu/sysemu.h | 4 ++--
migration/savevm.c | 51 ++++++++++++++++++++++++------------------------
replay/replay-snapshot.c | 6 ++++--
vl.c | 4 +++-
5 files changed, 41 insertions(+), 33 deletions(-)
diff --git a/hmp.c b/hmp.c
index bd7b1ca..d81f71e 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1273,17 +1273,22 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict)
{
int saved_vm_running = runstate_is_running();
const char *name = qdict_get_str(qdict, "name");
+ Error *err = NULL;
vm_stop(RUN_STATE_RESTORE_VM);
- if (load_vmstate(name) == 0 && saved_vm_running) {
+ if (load_vmstate(name, &err) == 0 && saved_vm_running) {
vm_start();
}
+ hmp_handle_error(mon, &err);
}
void hmp_savevm(Monitor *mon, const QDict *qdict)
{
- save_vmstate(qdict_get_try_str(qdict, "name"));
+ Error *err = NULL;
+
+ save_vmstate(qdict_get_try_str(qdict, "name"), &err);
+ hmp_handle_error(mon, &err);
}
void hmp_delvm(Monitor *mon, const QDict *qdict)
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 15656b7..058d5eb 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -75,8 +75,8 @@ void qemu_remove_exit_notifier(Notifier *notify);
void qemu_add_machine_init_done_notifier(Notifier *notify);
void qemu_remove_machine_init_done_notifier(Notifier *notify);
-int save_vmstate(const char *name);
-int load_vmstate(const char *name);
+int save_vmstate(const char *name, Error **errp);
+int load_vmstate(const char *name, Error **errp);
void qemu_announce_self(void);
diff --git a/migration/savevm.c b/migration/savevm.c
index 8dd4306..0c01988 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2066,7 +2066,7 @@ int qemu_loadvm_state(QEMUFile *f)
return ret;
}
-int save_vmstate(const char *name)
+int save_vmstate(const char *name, Error **errp)
{
BlockDriverState *bs, *bs1;
QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2076,29 +2076,27 @@ int save_vmstate(const char *name)
uint64_t vm_state_size;
qemu_timeval tv;
struct tm tm;
- Error *local_err = NULL;
AioContext *aio_context;
if (!bdrv_all_can_snapshot(&bs)) {
- error_report("Device '%s' is writable but does not support snapshots",
- bdrv_get_device_name(bs));
+ error_setg(errp, "Device '%s' is writable but does not support "
+ "snapshots", bdrv_get_device_name(bs));
return ret;
}
/* Delete old snapshots of the same name */
if (name) {
- ret = bdrv_all_delete_snapshot(name, &bs1, &local_err);
+ ret = bdrv_all_delete_snapshot(name, &bs1, errp);
if (ret < 0) {
- error_reportf_err(local_err,
- "Error while deleting snapshot on device '%s': ",
- bdrv_get_device_name(bs1));
+ error_prepend(errp, "Error while deleting snapshot on device "
+ "'%s': ", bdrv_get_device_name(bs1));
return ret;
}
}
bs = bdrv_all_find_vmstate_bs();
if (bs == NULL) {
- error_report("No block device can accept snapshots");
+ error_setg(errp, "No block device can accept snapshots");
return ret;
}
aio_context = bdrv_get_aio_context(bs);
@@ -2107,7 +2105,7 @@ int save_vmstate(const char *name)
ret = global_state_store();
if (ret) {
- error_report("Error saving global state");
+ error_setg(errp, "Error saving global state");
return ret;
}
vm_stop(RUN_STATE_SAVE_VM);
@@ -2139,21 +2137,20 @@ int save_vmstate(const char *name)
/* save the VM state */
f = qemu_fopen_bdrv(bs, 1);
if (!f) {
- error_report("Could not open VM state file");
+ error_setg(errp, "Could not open VM state file");
goto the_end;
}
- ret = qemu_savevm_state(f, &local_err);
+ ret = qemu_savevm_state(f, errp);
vm_state_size = qemu_ftell(f);
qemu_fclose(f);
if (ret < 0) {
- error_report_err(local_err);
goto the_end;
}
ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
if (ret < 0) {
- error_report("Error while creating snapshot on '%s'",
- bdrv_get_device_name(bs));
+ error_setg(errp, "Error while creating snapshot on '%s'",
+ bdrv_get_device_name(bs));
goto the_end;
}
@@ -2226,7 +2223,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
migration_incoming_state_destroy();
}
-int load_vmstate(const char *name)
+int load_vmstate(const char *name, Error **errp)
{
BlockDriverState *bs, *bs_vm_state;
QEMUSnapshotInfo sn;
@@ -2236,20 +2233,22 @@ int load_vmstate(const char *name)
MigrationIncomingState *mis = migration_incoming_get_current();
if (!bdrv_all_can_snapshot(&bs)) {
- error_report("Device '%s' is writable but does not support snapshots",
- bdrv_get_device_name(bs));
+ error_setg(errp,
+ "Device '%s' is writable but does not support snapshots",
+ bdrv_get_device_name(bs));
return -ENOTSUP;
}
ret = bdrv_all_find_snapshot(name, &bs);
if (ret < 0) {
- error_report("Device '%s' does not have the requested snapshot '%s'",
- bdrv_get_device_name(bs), name);
+ error_setg(errp,
+ "Device '%s' does not have the requested snapshot '%s'",
+ bdrv_get_device_name(bs), name);
return ret;
}
bs_vm_state = bdrv_all_find_vmstate_bs();
if (!bs_vm_state) {
- error_report("No block device supports snapshots");
+ error_setg(errp, "No block device supports snapshots");
return -ENOTSUP;
}
aio_context = bdrv_get_aio_context(bs_vm_state);
@@ -2261,8 +2260,8 @@ int load_vmstate(const char *name)
if (ret < 0) {
return ret;
} else if (sn.vm_state_size == 0) {
- error_report("This is a disk-only snapshot. Revert to it offline "
- "using qemu-img.");
+ error_setg(errp, "This is a disk-only snapshot. Revert to it "
+ " offline using qemu-img");
return -EINVAL;
}
@@ -2271,7 +2270,7 @@ int load_vmstate(const char *name)
ret = bdrv_all_goto_snapshot(name, &bs);
if (ret < 0) {
- error_report("Error %d while activating snapshot '%s' on '%s'",
+ error_setg(errp, "Error %d while activating snapshot '%s' on '%s'",
ret, name, bdrv_get_device_name(bs));
return ret;
}
@@ -2279,7 +2278,7 @@ int load_vmstate(const char *name)
/* restore the VM state */
f = qemu_fopen_bdrv(bs_vm_state, 0);
if (!f) {
- error_report("Could not open VM state file");
+ error_setg(errp, "Could not open VM state file");
return -EINVAL;
}
@@ -2293,7 +2292,7 @@ int load_vmstate(const char *name)
migration_incoming_state_destroy();
if (ret < 0) {
- error_report("Error %d while loading VM state", ret);
+ error_setg(errp, "Error %d while loading VM state", ret);
return ret;
}
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
index 8cced46..fdc4353 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -62,14 +62,16 @@ void replay_vmstate_register(void)
void replay_vmstate_init(void)
{
+ Error *err = NULL;
+
if (replay_snapshot) {
if (replay_mode == REPLAY_MODE_RECORD) {
- if (save_vmstate(replay_snapshot) != 0) {
+ if (save_vmstate(replay_snapshot, &err) != 0) {
error_report("Could not create snapshot for icount record");
exit(1);
}
} else if (replay_mode == REPLAY_MODE_PLAY) {
- if (load_vmstate(replay_snapshot) != 0) {
+ if (load_vmstate(replay_snapshot, &err) != 0) {
error_report("Could not load snapshot for icount replay");
exit(1);
}
diff --git a/vl.c b/vl.c
index 0b4ed52..8b3ec2e 100644
--- a/vl.c
+++ b/vl.c
@@ -4681,7 +4681,9 @@ int main(int argc, char **argv, char **envp)
if (replay_mode != REPLAY_MODE_NONE) {
replay_vmstate_init();
} else if (loadvm) {
- if (load_vmstate(loadvm) < 0) {
+ Error *local_err = NULL;
+ if (load_vmstate(loadvm, &local_err) < 0) {
+ error_report_err(local_err);
autostart = 0;
}
}
--
2.9.3
On 25/04/2017 12:24, Juan Quintela wrote:
> This way we use the "normal" way of printing errors for hmp commands.
>
> --
> Paolo suggestion
"Suggested-by" tag?
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> hmp.c | 9 +++++++--
> include/sysemu/sysemu.h | 4 ++--
> migration/savevm.c | 51 ++++++++++++++++++++++++------------------------
> replay/replay-snapshot.c | 6 ++++--
> vl.c | 4 +++-
> 5 files changed, 41 insertions(+), 33 deletions(-)
>
...
> diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
> index 8cced46..fdc4353 100644
> --- a/replay/replay-snapshot.c
> +++ b/replay/replay-snapshot.c
> @@ -62,14 +62,16 @@ void replay_vmstate_register(void)
>
> void replay_vmstate_init(void)
> {
> + Error *err = NULL;
> +
> if (replay_snapshot) {
> if (replay_mode == REPLAY_MODE_RECORD) {
> - if (save_vmstate(replay_snapshot) != 0) {
> + if (save_vmstate(replay_snapshot, &err) != 0) {
> error_report("Could not create snapshot for icount record");
> exit(1);
> }
> } else if (replay_mode == REPLAY_MODE_PLAY) {
> - if (load_vmstate(replay_snapshot) != 0) {
> + if (load_vmstate(replay_snapshot, &err) != 0) {
> error_report("Could not load snapshot for icount replay");
> exit(1);
> }
You can use "&error_fatal" in these cases.
Thanks,
Laurent
* Juan Quintela (quintela@redhat.com) wrote:
> This way we use the "normal" way of printing errors for hmp commands.
>
> --
> Paolo suggestion
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
<snip>
> +int save_vmstate(const char *name, Error **errp)
> {
> BlockDriverState *bs, *bs1;
> QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> @@ -2076,29 +2076,27 @@ int save_vmstate(const char *name)
> uint64_t vm_state_size;
> qemu_timeval tv;
> struct tm tm;
> - Error *local_err = NULL;
> AioContext *aio_context;
>
> if (!bdrv_all_can_snapshot(&bs)) {
> - error_report("Device '%s' is writable but does not support snapshots",
> - bdrv_get_device_name(bs));
> + error_setg(errp, "Device '%s' is writable but does not support "
> + "snapshots", bdrv_get_device_name(bs));
> return ret;
> }
>
> /* Delete old snapshots of the same name */
> if (name) {
> - ret = bdrv_all_delete_snapshot(name, &bs1, &local_err);
> + ret = bdrv_all_delete_snapshot(name, &bs1, errp);
> if (ret < 0) {
> - error_reportf_err(local_err,
> - "Error while deleting snapshot on device '%s': ",
> - bdrv_get_device_name(bs1));
> + error_prepend(errp, "Error while deleting snapshot on device "
> + "'%s': ", bdrv_get_device_name(bs1));
I thought the rule was that you had to use a local_err and use error_propagate?
(I hate that rule)
> return ret;
> }
> }
>
> bs = bdrv_all_find_vmstate_bs();
> if (bs == NULL) {
> - error_report("No block device can accept snapshots");
> + error_setg(errp, "No block device can accept snapshots");
> return ret;
> }
> aio_context = bdrv_get_aio_context(bs);
> @@ -2107,7 +2105,7 @@ int save_vmstate(const char *name)
>
> ret = global_state_store();
> if (ret) {
> - error_report("Error saving global state");
> + error_setg(errp, "Error saving global state");
> return ret;
> }
> vm_stop(RUN_STATE_SAVE_VM);
> @@ -2139,21 +2137,20 @@ int save_vmstate(const char *name)
> /* save the VM state */
> f = qemu_fopen_bdrv(bs, 1);
> if (!f) {
> - error_report("Could not open VM state file");
> + error_setg(errp, "Could not open VM state file");
> goto the_end;
> }
> - ret = qemu_savevm_state(f, &local_err);
> + ret = qemu_savevm_state(f, errp);
> vm_state_size = qemu_ftell(f);
> qemu_fclose(f);
> if (ret < 0) {
> - error_report_err(local_err);
> goto the_end;
> }
>
> ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
> if (ret < 0) {
> - error_report("Error while creating snapshot on '%s'",
> - bdrv_get_device_name(bs));
> + error_setg(errp, "Error while creating snapshot on '%s'",
> + bdrv_get_device_name(bs));
> goto the_end;
> }
>
> @@ -2226,7 +2223,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
> migration_incoming_state_destroy();
> }
>
> -int load_vmstate(const char *name)
> +int load_vmstate(const char *name, Error **errp)
> {
> BlockDriverState *bs, *bs_vm_state;
> QEMUSnapshotInfo sn;
> @@ -2236,20 +2233,22 @@ int load_vmstate(const char *name)
> MigrationIncomingState *mis = migration_incoming_get_current();
>
> if (!bdrv_all_can_snapshot(&bs)) {
> - error_report("Device '%s' is writable but does not support snapshots",
> - bdrv_get_device_name(bs));
> + error_setg(errp,
> + "Device '%s' is writable but does not support snapshots",
> + bdrv_get_device_name(bs));
> return -ENOTSUP;
> }
> ret = bdrv_all_find_snapshot(name, &bs);
> if (ret < 0) {
> - error_report("Device '%s' does not have the requested snapshot '%s'",
> - bdrv_get_device_name(bs), name);
> + error_setg(errp,
> + "Device '%s' does not have the requested snapshot '%s'",
> + bdrv_get_device_name(bs), name);
> return ret;
> }
>
> bs_vm_state = bdrv_all_find_vmstate_bs();
> if (!bs_vm_state) {
> - error_report("No block device supports snapshots");
> + error_setg(errp, "No block device supports snapshots");
> return -ENOTSUP;
> }
> aio_context = bdrv_get_aio_context(bs_vm_state);
> @@ -2261,8 +2260,8 @@ int load_vmstate(const char *name)
> if (ret < 0) {
> return ret;
> } else if (sn.vm_state_size == 0) {
> - error_report("This is a disk-only snapshot. Revert to it offline "
> - "using qemu-img.");
> + error_setg(errp, "This is a disk-only snapshot. Revert to it "
> + " offline using qemu-img");
> return -EINVAL;
> }
>
> @@ -2271,7 +2270,7 @@ int load_vmstate(const char *name)
>
> ret = bdrv_all_goto_snapshot(name, &bs);
> if (ret < 0) {
> - error_report("Error %d while activating snapshot '%s' on '%s'",
> + error_setg(errp, "Error %d while activating snapshot '%s' on '%s'",
> ret, name, bdrv_get_device_name(bs));
> return ret;
> }
> @@ -2279,7 +2278,7 @@ int load_vmstate(const char *name)
> /* restore the VM state */
> f = qemu_fopen_bdrv(bs_vm_state, 0);
> if (!f) {
> - error_report("Could not open VM state file");
> + error_setg(errp, "Could not open VM state file");
> return -EINVAL;
> }
>
> @@ -2293,7 +2292,7 @@ int load_vmstate(const char *name)
>
> migration_incoming_state_destroy();
> if (ret < 0) {
> - error_report("Error %d while loading VM state", ret);
> + error_setg(errp, "Error %d while loading VM state", ret);
> return ret;
> }
>
> diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
> index 8cced46..fdc4353 100644
> --- a/replay/replay-snapshot.c
> +++ b/replay/replay-snapshot.c
> @@ -62,14 +62,16 @@ void replay_vmstate_register(void)
>
> void replay_vmstate_init(void)
> {
> + Error *err = NULL;
> +
> if (replay_snapshot) {
> if (replay_mode == REPLAY_MODE_RECORD) {
> - if (save_vmstate(replay_snapshot) != 0) {
> + if (save_vmstate(replay_snapshot, &err) != 0) {
> error_report("Could not create snapshot for icount record");
> exit(1);
> }
> } else if (replay_mode == REPLAY_MODE_PLAY) {
> - if (load_vmstate(replay_snapshot) != 0) {
> + if (load_vmstate(replay_snapshot, &err) != 0) {
> error_report("Could not load snapshot for icount replay");
> exit(1);
> }
How do either of the contents of the 'err' get reported - they're not
printed at all are they?
(I don't like error_abort, I'd rather see the other message as well).
Dave
> diff --git a/vl.c b/vl.c
> index 0b4ed52..8b3ec2e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4681,7 +4681,9 @@ int main(int argc, char **argv, char **envp)
> if (replay_mode != REPLAY_MODE_NONE) {
> replay_vmstate_init();
> } else if (loadvm) {
> - if (load_vmstate(loadvm) < 0) {
> + Error *local_err = NULL;
> + if (load_vmstate(loadvm, &local_err) < 0) {
> + error_report_err(local_err);
> autostart = 0;
> }
> }
> --
> 2.9.3
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 04/28/2017 09:47 AM, Dr. David Alan Gilbert wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> This way we use the "normal" way of printing errors for hmp commands.
>>
>> --
>> if (!bdrv_all_can_snapshot(&bs)) {
>> - error_report("Device '%s' is writable but does not support snapshots",
>> - bdrv_get_device_name(bs));
>> + error_setg(errp, "Device '%s' is writable but does not support "
>> + "snapshots", bdrv_get_device_name(bs));
>> return ret;
>> }
>>
>> /* Delete old snapshots of the same name */
>> if (name) {
>> - ret = bdrv_all_delete_snapshot(name, &bs1, &local_err);
>> + ret = bdrv_all_delete_snapshot(name, &bs1, errp);
>> if (ret < 0) {
>> - error_reportf_err(local_err,
>> - "Error while deleting snapshot on device '%s': ",
>> - bdrv_get_device_name(bs1));
>> + error_prepend(errp, "Error while deleting snapshot on device "
>> + "'%s': ", bdrv_get_device_name(bs1));
>
> I thought the rule was that you had to use a local_err and use error_propagate?
> (I hate that rule)
The rule is that if you want to make code conditional on whether an
error occurred, you can't do it by checking errp (because the caller may
have passed NULL), so you instead have to use local_err and
error_propagate(). But if there are other ways to tell if an error
occurred (such as a return value), then passing errp directly through is
fine (error_prepend does the right thing even if errp is NULL because
the caller doesn't care about an error).
include/qapi/error.h has some good examples, and if it is missing
something, we'd be glad to patch it further to serve as a good reference.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
© 2016 - 2026 Red Hat, Inc.