From: Philippe Mathieu-Daudé <philmd@redhat.com>
Just for consistency, following the example documented since
commit e3fe3988d7 ("error: Document Error API usage rules"),
return a boolean value indicating an error is set or not.
Acked-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
include/migration/snapshot.h | 9 ++++++++-
migration/savevm.c | 16 ++++++++--------
replay/replay-debugging.c | 2 +-
replay/replay-snapshot.c | 2 +-
4 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h
index c85b6ec75b..0eaf1ba0b1 100644
--- a/include/migration/snapshot.h
+++ b/include/migration/snapshot.h
@@ -15,7 +15,14 @@
#ifndef QEMU_MIGRATION_SNAPSHOT_H
#define QEMU_MIGRATION_SNAPSHOT_H
-int save_snapshot(const char *name, Error **errp);
+/**
+ * save_snapshot: Save an internal snapshot.
+ * @name: name of internal snapshot
+ * @errp: pointer to error object
+ * On success, return %true.
+ * On failure, store an error through @errp and return %false.
+ */
+bool save_snapshot(const char *name, Error **errp);
int load_snapshot(const char *name, Error **errp);
#endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 4a7237337e..ef7963f6c9 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2729,7 +2729,7 @@ int qemu_load_device_state(QEMUFile *f)
return 0;
}
-int save_snapshot(const char *name, Error **errp)
+bool save_snapshot(const char *name, Error **errp)
{
BlockDriverState *bs;
QEMUSnapshotInfo sn1, *sn = &sn1;
@@ -2742,29 +2742,29 @@ int save_snapshot(const char *name, Error **errp)
AioContext *aio_context;
if (migration_is_blocked(errp)) {
- return ret;
+ return false;
}
if (!replay_can_snapshot()) {
error_setg(errp, "Record/replay does not allow making snapshot "
"right now. Try once more later.");
- return ret;
+ return false;
}
if (!bdrv_all_can_snapshot(errp)) {
- return ret;
+ return false;
}
/* Delete old snapshots of the same name */
if (name) {
if (bdrv_all_delete_snapshot(name, errp) < 0) {
- return ret;
+ return false;
}
}
bs = bdrv_all_find_vmstate_bs(errp);
if (bs == NULL) {
- return ret;
+ return false;
}
aio_context = bdrv_get_aio_context(bs);
@@ -2773,7 +2773,7 @@ int save_snapshot(const char *name, Error **errp)
ret = global_state_store();
if (ret) {
error_setg(errp, "Error saving global state");
- return ret;
+ return false;
}
vm_stop(RUN_STATE_SAVE_VM);
@@ -2845,7 +2845,7 @@ int save_snapshot(const char *name, Error **errp)
if (saved_vm_running) {
vm_start();
}
- return ret;
+ return ret == 0;
}
void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
diff --git a/replay/replay-debugging.c b/replay/replay-debugging.c
index 3a9b609e62..8e0050915d 100644
--- a/replay/replay-debugging.c
+++ b/replay/replay-debugging.c
@@ -323,7 +323,7 @@ void replay_gdb_attached(void)
*/
if (replay_mode == REPLAY_MODE_PLAY
&& !replay_snapshot) {
- if (save_snapshot("start_debugging", NULL) != 0) {
+ if (!save_snapshot("start_debugging", NULL)) {
/* Can't create the snapshot. Continue conventional debugging. */
}
}
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
index e26fa4c892..4f2560d156 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -77,7 +77,7 @@ void replay_vmstate_init(void)
if (replay_snapshot) {
if (replay_mode == REPLAY_MODE_RECORD) {
- if (save_snapshot(replay_snapshot, &err) != 0) {
+ if (!save_snapshot(replay_snapshot, &err)) {
error_report_err(err);
error_report("Could not create snapshot for icount record");
exit(1);
--
2.29.2
On 1/20/21 4:44 AM, Daniel P. Berrangé wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> Just for consistency, following the example documented since
> commit e3fe3988d7 ("error: Document Error API usage rules"),
> return a boolean value indicating an error is set or not.
>
> Acked-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> include/migration/snapshot.h | 9 ++++++++-
> migration/savevm.c | 16 ++++++++--------
> replay/replay-debugging.c | 2 +-
> replay/replay-snapshot.c | 2 +-
> 4 files changed, 18 insertions(+), 11 deletions(-)
> +++ b/migration/savevm.c
> @@ -2729,7 +2729,7 @@ int qemu_load_device_state(QEMUFile *f)
> return 0;
> }
>
> -int save_snapshot(const char *name, Error **errp)
> +bool save_snapshot(const char *name, Error **errp)
> {
Missing a 'return -1' that must be changed to 'return false' (see patch
11; if not fixed here, you have a window where you are inadvertently
returning true on failure).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 1/20/21 12:46 PM, Eric Blake wrote:
> On 1/20/21 4:44 AM, Daniel P. Berrangé wrote:
>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>> Just for consistency, following the example documented since
>> commit e3fe3988d7 ("error: Document Error API usage rules"),
>> return a boolean value indicating an error is set or not.
>>
>> Acked-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> include/migration/snapshot.h | 9 ++++++++-
>> migration/savevm.c | 16 ++++++++--------
>> replay/replay-debugging.c | 2 +-
>> replay/replay-snapshot.c | 2 +-
>> 4 files changed, 18 insertions(+), 11 deletions(-)
>
>
>> +++ b/migration/savevm.c
>> @@ -2729,7 +2729,7 @@ int qemu_load_device_state(QEMUFile *f)
>> return 0;
>> }
>>
>> -int save_snapshot(const char *name, Error **errp)
>> +bool save_snapshot(const char *name, Error **errp)
>> {
>
> Missing a 'return -1' that must be changed to 'return false' (see patch
> 11; if not fixed here, you have a window where you are inadvertently
> returning true on failure).
>
Correction, it was patch 6/11 that introduced the 'return -1' that then
got fixed in 11/11; patch 2/11 by itself is okay.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
* Daniel P. Berrangé (berrange@redhat.com) wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> Just for consistency, following the example documented since
> commit e3fe3988d7 ("error: Document Error API usage rules"),
> return a boolean value indicating an error is set or not.
>
> Acked-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> include/migration/snapshot.h | 9 ++++++++-
> migration/savevm.c | 16 ++++++++--------
> replay/replay-debugging.c | 2 +-
> replay/replay-snapshot.c | 2 +-
> 4 files changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h
> index c85b6ec75b..0eaf1ba0b1 100644
> --- a/include/migration/snapshot.h
> +++ b/include/migration/snapshot.h
> @@ -15,7 +15,14 @@
> #ifndef QEMU_MIGRATION_SNAPSHOT_H
> #define QEMU_MIGRATION_SNAPSHOT_H
>
> -int save_snapshot(const char *name, Error **errp);
> +/**
> + * save_snapshot: Save an internal snapshot.
> + * @name: name of internal snapshot
> + * @errp: pointer to error object
> + * On success, return %true.
> + * On failure, store an error through @errp and return %false.
> + */
> +bool save_snapshot(const char *name, Error **errp);
> int load_snapshot(const char *name, Error **errp);
>
> #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 4a7237337e..ef7963f6c9 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2729,7 +2729,7 @@ int qemu_load_device_state(QEMUFile *f)
> return 0;
> }
>
> -int save_snapshot(const char *name, Error **errp)
> +bool save_snapshot(const char *name, Error **errp)
> {
> BlockDriverState *bs;
> QEMUSnapshotInfo sn1, *sn = &sn1;
> @@ -2742,29 +2742,29 @@ int save_snapshot(const char *name, Error **errp)
> AioContext *aio_context;
>
> if (migration_is_blocked(errp)) {
> - return ret;
> + return false;
> }
>
> if (!replay_can_snapshot()) {
> error_setg(errp, "Record/replay does not allow making snapshot "
> "right now. Try once more later.");
> - return ret;
> + return false;
> }
>
> if (!bdrv_all_can_snapshot(errp)) {
> - return ret;
> + return false;
> }
>
> /* Delete old snapshots of the same name */
> if (name) {
> if (bdrv_all_delete_snapshot(name, errp) < 0) {
> - return ret;
> + return false;
> }
> }
>
> bs = bdrv_all_find_vmstate_bs(errp);
> if (bs == NULL) {
> - return ret;
> + return false;
> }
> aio_context = bdrv_get_aio_context(bs);
>
> @@ -2773,7 +2773,7 @@ int save_snapshot(const char *name, Error **errp)
> ret = global_state_store();
> if (ret) {
> error_setg(errp, "Error saving global state");
> - return ret;
> + return false;
> }
> vm_stop(RUN_STATE_SAVE_VM);
>
> @@ -2845,7 +2845,7 @@ int save_snapshot(const char *name, Error **errp)
> if (saved_vm_running) {
> vm_start();
> }
> - return ret;
> + return ret == 0;
> }
>
> void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
> diff --git a/replay/replay-debugging.c b/replay/replay-debugging.c
> index 3a9b609e62..8e0050915d 100644
> --- a/replay/replay-debugging.c
> +++ b/replay/replay-debugging.c
> @@ -323,7 +323,7 @@ void replay_gdb_attached(void)
> */
> if (replay_mode == REPLAY_MODE_PLAY
> && !replay_snapshot) {
> - if (save_snapshot("start_debugging", NULL) != 0) {
> + if (!save_snapshot("start_debugging", NULL)) {
> /* Can't create the snapshot. Continue conventional debugging. */
> }
> }
> diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
> index e26fa4c892..4f2560d156 100644
> --- a/replay/replay-snapshot.c
> +++ b/replay/replay-snapshot.c
> @@ -77,7 +77,7 @@ void replay_vmstate_init(void)
>
> if (replay_snapshot) {
> if (replay_mode == REPLAY_MODE_RECORD) {
> - if (save_snapshot(replay_snapshot, &err) != 0) {
> + if (!save_snapshot(replay_snapshot, &err)) {
> error_report_err(err);
> error_report("Could not create snapshot for icount record");
> exit(1);
> --
> 2.29.2
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
© 2016 - 2025 Red Hat, Inc.