* [Qemu-devel] [PATCH 1/6] monitor: Remove monitor parameter from save_vmstate
2017-04-25 10:24 [Qemu-devel] [PATCH 0/6] Move snapshots commands to hmp Juan Quintela
@ 2017-04-25 10:24 ` Juan Quintela
2017-04-25 13:27 ` Laurent Vivier
2017-04-25 10:24 ` [Qemu-devel] [PATCH 2/6] monitor: Move hmp_loadvm from monitor.c to hmp.c Juan Quintela
` (5 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Juan Quintela @ 2017-04-25 10:24 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx
load_vmstate() already use error_report, so be consistent. There is
an identical error message in load_vmstate() that ends in a
period. Remove it.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
include/sysemu/sysemu.h | 2 +-
migration/savevm.c | 20 ++++++++++----------
replay/replay-snapshot.c | 2 +-
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 16175f7..b6daf9d 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -76,7 +76,7 @@ void qemu_add_machine_init_done_notifier(Notifier *notify);
void qemu_remove_machine_init_done_notifier(Notifier *notify);
void hmp_savevm(Monitor *mon, const QDict *qdict);
-int save_vmstate(Monitor *mon, const char *name);
+int save_vmstate(const char *name);
int load_vmstate(const char *name);
void hmp_delvm(Monitor *mon, const QDict *qdict);
void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
diff --git a/migration/savevm.c b/migration/savevm.c
index 7421a67..ff934aa 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2070,7 +2070,7 @@ int qemu_loadvm_state(QEMUFile *f)
return ret;
}
-int save_vmstate(Monitor *mon, const char *name)
+int save_vmstate(const char *name)
{
BlockDriverState *bs, *bs1;
QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2084,8 +2084,8 @@ int save_vmstate(Monitor *mon, const char *name)
AioContext *aio_context;
if (!bdrv_all_can_snapshot(&bs)) {
- monitor_printf(mon, "Device '%s' is writable but does not "
- "support snapshots.\n", bdrv_get_device_name(bs));
+ error_report("Device '%s' is writable but does not support snapshots",
+ bdrv_get_device_name(bs));
return ret;
}
@@ -2102,7 +2102,7 @@ int save_vmstate(Monitor *mon, const char *name)
bs = bdrv_all_find_vmstate_bs();
if (bs == NULL) {
- monitor_printf(mon, "No block device can accept snapshots\n");
+ error_report("No block device can accept snapshots");
return ret;
}
aio_context = bdrv_get_aio_context(bs);
@@ -2111,7 +2111,7 @@ int save_vmstate(Monitor *mon, const char *name)
ret = global_state_store();
if (ret) {
- monitor_printf(mon, "Error saving global state\n");
+ error_report("Error saving global state");
return ret;
}
vm_stop(RUN_STATE_SAVE_VM);
@@ -2143,7 +2143,7 @@ int save_vmstate(Monitor *mon, const char *name)
/* save the VM state */
f = qemu_fopen_bdrv(bs, 1);
if (!f) {
- monitor_printf(mon, "Could not open VM state file\n");
+ error_report("Could not open VM state file");
goto the_end;
}
ret = qemu_savevm_state(f, &local_err);
@@ -2156,8 +2156,8 @@ int save_vmstate(Monitor *mon, const char *name)
ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
if (ret < 0) {
- monitor_printf(mon, "Error while creating snapshot on '%s'\n",
- bdrv_get_device_name(bs));
+ error_report("Error while creating snapshot on '%s'",
+ bdrv_get_device_name(bs));
goto the_end;
}
@@ -2173,7 +2173,7 @@ int save_vmstate(Monitor *mon, const char *name)
void hmp_savevm(Monitor *mon, const QDict *qdict)
{
- save_vmstate(mon, qdict_get_try_str(qdict, "name"));
+ save_vmstate(qdict_get_try_str(qdict, "name"));
}
void qmp_xen_save_devices_state(const char *filename, Error **errp)
@@ -2245,7 +2245,7 @@ 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.",
+ error_report("Device '%s' is writable but does not support snapshots",
bdrv_get_device_name(bs));
return -ENOTSUP;
}
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
index 65e2d37..8cced46 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -64,7 +64,7 @@ void replay_vmstate_init(void)
{
if (replay_snapshot) {
if (replay_mode == REPLAY_MODE_RECORD) {
- if (save_vmstate(cur_mon, replay_snapshot) != 0) {
+ if (save_vmstate(replay_snapshot) != 0) {
error_report("Could not create snapshot for icount record");
exit(1);
}
--
2.9.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 1/6] monitor: Remove monitor parameter from save_vmstate
2017-04-25 10:24 ` [Qemu-devel] [PATCH 1/6] monitor: Remove monitor parameter from save_vmstate Juan Quintela
@ 2017-04-25 13:27 ` Laurent Vivier
0 siblings, 0 replies; 22+ messages in thread
From: Laurent Vivier @ 2017-04-25 13:27 UTC (permalink / raw)
To: Juan Quintela, qemu-devel; +Cc: dgilbert, peterx
On 25/04/2017 12:24, Juan Quintela wrote:
> load_vmstate() already use error_report, so be consistent. There is
> an identical error message in load_vmstate() that ends in a
> period. Remove it.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> ---
> include/sysemu/sysemu.h | 2 +-
> migration/savevm.c | 20 ++++++++++----------
> replay/replay-snapshot.c | 2 +-
> 3 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 16175f7..b6daf9d 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -76,7 +76,7 @@ void qemu_add_machine_init_done_notifier(Notifier *notify);
> void qemu_remove_machine_init_done_notifier(Notifier *notify);
>
> void hmp_savevm(Monitor *mon, const QDict *qdict);
> -int save_vmstate(Monitor *mon, const char *name);
> +int save_vmstate(const char *name);
> int load_vmstate(const char *name);
> void hmp_delvm(Monitor *mon, const QDict *qdict);
> void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 7421a67..ff934aa 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2070,7 +2070,7 @@ int qemu_loadvm_state(QEMUFile *f)
> return ret;
> }
>
> -int save_vmstate(Monitor *mon, const char *name)
> +int save_vmstate(const char *name)
> {
> BlockDriverState *bs, *bs1;
> QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> @@ -2084,8 +2084,8 @@ int save_vmstate(Monitor *mon, const char *name)
> AioContext *aio_context;
>
> if (!bdrv_all_can_snapshot(&bs)) {
> - monitor_printf(mon, "Device '%s' is writable but does not "
> - "support snapshots.\n", bdrv_get_device_name(bs));
> + error_report("Device '%s' is writable but does not support snapshots",
> + bdrv_get_device_name(bs));
> return ret;
> }
>
> @@ -2102,7 +2102,7 @@ int save_vmstate(Monitor *mon, const char *name)
>
> bs = bdrv_all_find_vmstate_bs();
> if (bs == NULL) {
> - monitor_printf(mon, "No block device can accept snapshots\n");
> + error_report("No block device can accept snapshots");
> return ret;
> }
> aio_context = bdrv_get_aio_context(bs);
> @@ -2111,7 +2111,7 @@ int save_vmstate(Monitor *mon, const char *name)
>
> ret = global_state_store();
> if (ret) {
> - monitor_printf(mon, "Error saving global state\n");
> + error_report("Error saving global state");
> return ret;
> }
> vm_stop(RUN_STATE_SAVE_VM);
> @@ -2143,7 +2143,7 @@ int save_vmstate(Monitor *mon, const char *name)
> /* save the VM state */
> f = qemu_fopen_bdrv(bs, 1);
> if (!f) {
> - monitor_printf(mon, "Could not open VM state file\n");
> + error_report("Could not open VM state file");
> goto the_end;
> }
> ret = qemu_savevm_state(f, &local_err);
> @@ -2156,8 +2156,8 @@ int save_vmstate(Monitor *mon, const char *name)
>
> ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
> if (ret < 0) {
> - monitor_printf(mon, "Error while creating snapshot on '%s'\n",
> - bdrv_get_device_name(bs));
> + error_report("Error while creating snapshot on '%s'",
> + bdrv_get_device_name(bs));
> goto the_end;
> }
>
> @@ -2173,7 +2173,7 @@ int save_vmstate(Monitor *mon, const char *name)
>
> void hmp_savevm(Monitor *mon, const QDict *qdict)
> {
> - save_vmstate(mon, qdict_get_try_str(qdict, "name"));
> + save_vmstate(qdict_get_try_str(qdict, "name"));
> }
>
> void qmp_xen_save_devices_state(const char *filename, Error **errp)
> @@ -2245,7 +2245,7 @@ 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.",
> + error_report("Device '%s' is writable but does not support snapshots",
> bdrv_get_device_name(bs));
> return -ENOTSUP;
> }
> diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
> index 65e2d37..8cced46 100644
> --- a/replay/replay-snapshot.c
> +++ b/replay/replay-snapshot.c
> @@ -64,7 +64,7 @@ void replay_vmstate_init(void)
> {
> if (replay_snapshot) {
> if (replay_mode == REPLAY_MODE_RECORD) {
> - if (save_vmstate(cur_mon, replay_snapshot) != 0) {
> + if (save_vmstate(replay_snapshot) != 0) {
> error_report("Could not create snapshot for icount record");
> exit(1);
> }
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 2/6] monitor: Move hmp_loadvm from monitor.c to hmp.c
2017-04-25 10:24 [Qemu-devel] [PATCH 0/6] Move snapshots commands to hmp Juan Quintela
2017-04-25 10:24 ` [Qemu-devel] [PATCH 1/6] monitor: Remove monitor parameter from save_vmstate Juan Quintela
@ 2017-04-25 10:24 ` Juan Quintela
2017-04-25 13:37 ` Laurent Vivier
2017-04-25 10:24 ` [Qemu-devel] [PATCH 3/6] monitor: Move hmp_savevm from savevm.c " Juan Quintela
` (4 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Juan Quintela @ 2017-04-25 10:24 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx
We are going to move the rest of hmp snapshots functions there instead
of monitor.c.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
hmp.c | 13 +++++++++++++
hmp.h | 1 +
monitor.c | 13 -------------
3 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/hmp.c b/hmp.c
index ab407d6..f6b8738 100644
--- a/hmp.c
+++ b/hmp.c
@@ -19,6 +19,7 @@
#include "net/eth.h"
#include "sysemu/char.h"
#include "sysemu/block-backend.h"
+#include "sysemu/sysemu.h"
#include "qemu/config-file.h"
#include "qemu/option.h"
#include "qemu/timer.h"
@@ -1268,6 +1269,18 @@ void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, &err);
}
+void hmp_loadvm(Monitor *mon, const QDict *qdict)
+{
+ int saved_vm_running = runstate_is_running();
+ const char *name = qdict_get_str(qdict, "name");
+
+ vm_stop(RUN_STATE_RESTORE_VM);
+
+ if (load_vmstate(name) == 0 && saved_vm_running) {
+ vm_start();
+ }
+}
+
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
{
qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index 799fd37..385332c 100644
--- a/hmp.h
+++ b/hmp.h
@@ -63,6 +63,7 @@ void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict);
void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
void hmp_drive_backup(Monitor *mon, const QDict *qdict);
+void hmp_loadvm(Monitor *mon, const QDict *qdict);
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
diff --git a/monitor.c b/monitor.c
index be282ec..d02900d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -37,7 +37,6 @@
#include "net/slirp.h"
#include "sysemu/char.h"
#include "ui/qemu-spice.h"
-#include "sysemu/sysemu.h"
#include "sysemu/numa.h"
#include "monitor/monitor.h"
#include "qemu/config-file.h"
@@ -1843,18 +1842,6 @@ void qmp_closefd(const char *fdname, Error **errp)
error_setg(errp, QERR_FD_NOT_FOUND, fdname);
}
-static void hmp_loadvm(Monitor *mon, const QDict *qdict)
-{
- int saved_vm_running = runstate_is_running();
- const char *name = qdict_get_str(qdict, "name");
-
- vm_stop(RUN_STATE_RESTORE_VM);
-
- if (load_vmstate(name) == 0 && saved_vm_running) {
- vm_start();
- }
-}
-
int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
{
mon_fd_t *monfd;
--
2.9.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 2/6] monitor: Move hmp_loadvm from monitor.c to hmp.c
2017-04-25 10:24 ` [Qemu-devel] [PATCH 2/6] monitor: Move hmp_loadvm from monitor.c to hmp.c Juan Quintela
@ 2017-04-25 13:37 ` Laurent Vivier
0 siblings, 0 replies; 22+ messages in thread
From: Laurent Vivier @ 2017-04-25 13:37 UTC (permalink / raw)
To: Juan Quintela, qemu-devel; +Cc: dgilbert, peterx
On 25/04/2017 12:24, Juan Quintela wrote:
> We are going to move the rest of hmp snapshots functions there instead
> of monitor.c.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> ---
> hmp.c | 13 +++++++++++++
> hmp.h | 1 +
> monitor.c | 13 -------------
> 3 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index ab407d6..f6b8738 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -19,6 +19,7 @@
> #include "net/eth.h"
> #include "sysemu/char.h"
> #include "sysemu/block-backend.h"
> +#include "sysemu/sysemu.h"
> #include "qemu/config-file.h"
> #include "qemu/option.h"
> #include "qemu/timer.h"
> @@ -1268,6 +1269,18 @@ void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict)
> hmp_handle_error(mon, &err);
> }
>
> +void hmp_loadvm(Monitor *mon, const QDict *qdict)
> +{
> + int saved_vm_running = runstate_is_running();
> + const char *name = qdict_get_str(qdict, "name");
> +
> + vm_stop(RUN_STATE_RESTORE_VM);
> +
> + if (load_vmstate(name) == 0 && saved_vm_running) {
> + vm_start();
> + }
> +}
> +
> void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
> {
> qmp_migrate_cancel(NULL);
> diff --git a/hmp.h b/hmp.h
> index 799fd37..385332c 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -63,6 +63,7 @@ void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict);
> void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
> void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
> void hmp_drive_backup(Monitor *mon, const QDict *qdict);
> +void hmp_loadvm(Monitor *mon, const QDict *qdict);
> void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
> void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
> void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
> diff --git a/monitor.c b/monitor.c
> index be282ec..d02900d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -37,7 +37,6 @@
> #include "net/slirp.h"
> #include "sysemu/char.h"
> #include "ui/qemu-spice.h"
> -#include "sysemu/sysemu.h"
> #include "sysemu/numa.h"
> #include "monitor/monitor.h"
> #include "qemu/config-file.h"
> @@ -1843,18 +1842,6 @@ void qmp_closefd(const char *fdname, Error **errp)
> error_setg(errp, QERR_FD_NOT_FOUND, fdname);
> }
>
> -static void hmp_loadvm(Monitor *mon, const QDict *qdict)
> -{
> - int saved_vm_running = runstate_is_running();
> - const char *name = qdict_get_str(qdict, "name");
> -
> - vm_stop(RUN_STATE_RESTORE_VM);
> -
> - if (load_vmstate(name) == 0 && saved_vm_running) {
> - vm_start();
> - }
> -}
> -
> int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
> {
> mon_fd_t *monfd;
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 3/6] monitor: Move hmp_savevm from savevm.c to hmp.c
2017-04-25 10:24 [Qemu-devel] [PATCH 0/6] Move snapshots commands to hmp Juan Quintela
2017-04-25 10:24 ` [Qemu-devel] [PATCH 1/6] monitor: Remove monitor parameter from save_vmstate Juan Quintela
2017-04-25 10:24 ` [Qemu-devel] [PATCH 2/6] monitor: Move hmp_loadvm from monitor.c to hmp.c Juan Quintela
@ 2017-04-25 10:24 ` Juan Quintela
2017-04-25 13:33 ` Laurent Vivier
2017-04-25 10:24 ` [Qemu-devel] [PATCH 4/6] monitor: Move hmp_delvm " Juan Quintela
` (3 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Juan Quintela @ 2017-04-25 10:24 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx
It is a monitor command, and has nothing migration specific in it.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
hmp.c | 5 +++++
hmp.h | 1 +
include/sysemu/sysemu.h | 1 -
migration/savevm.c | 5 -----
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/hmp.c b/hmp.c
index f6b8738..a82a952 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1281,6 +1281,11 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict)
}
}
+void hmp_savevm(Monitor *mon, const QDict *qdict)
+{
+ save_vmstate(qdict_get_try_str(qdict, "name"));
+}
+
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
{
qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index 385332c..b302c8d 100644
--- a/hmp.h
+++ b/hmp.h
@@ -64,6 +64,7 @@ void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
void hmp_drive_backup(Monitor *mon, const QDict *qdict);
void hmp_loadvm(Monitor *mon, const QDict *qdict);
+void hmp_savevm(Monitor *mon, const QDict *qdict);
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b6daf9d..914c36c 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -75,7 +75,6 @@ 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);
-void hmp_savevm(Monitor *mon, const QDict *qdict);
int save_vmstate(const char *name);
int load_vmstate(const char *name);
void hmp_delvm(Monitor *mon, const QDict *qdict);
diff --git a/migration/savevm.c b/migration/savevm.c
index ff934aa..bbff4d8 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2171,11 +2171,6 @@ int save_vmstate(const char *name)
return ret;
}
-void hmp_savevm(Monitor *mon, const QDict *qdict)
-{
- save_vmstate(qdict_get_try_str(qdict, "name"));
-}
-
void qmp_xen_save_devices_state(const char *filename, Error **errp)
{
QEMUFile *f;
--
2.9.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 3/6] monitor: Move hmp_savevm from savevm.c to hmp.c
2017-04-25 10:24 ` [Qemu-devel] [PATCH 3/6] monitor: Move hmp_savevm from savevm.c " Juan Quintela
@ 2017-04-25 13:33 ` Laurent Vivier
0 siblings, 0 replies; 22+ messages in thread
From: Laurent Vivier @ 2017-04-25 13:33 UTC (permalink / raw)
To: Juan Quintela, qemu-devel; +Cc: dgilbert, peterx
On 25/04/2017 12:24, Juan Quintela wrote:
> It is a monitor command, and has nothing migration specific in it.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> ---
> hmp.c | 5 +++++
> hmp.h | 1 +
> include/sysemu/sysemu.h | 1 -
> migration/savevm.c | 5 -----
> 4 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index f6b8738..a82a952 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1281,6 +1281,11 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict)
> }
> }
>
> +void hmp_savevm(Monitor *mon, const QDict *qdict)
> +{
> + save_vmstate(qdict_get_try_str(qdict, "name"));
> +}
> +
> void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
> {
> qmp_migrate_cancel(NULL);
> diff --git a/hmp.h b/hmp.h
> index 385332c..b302c8d 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -64,6 +64,7 @@ void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
> void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
> void hmp_drive_backup(Monitor *mon, const QDict *qdict);
> void hmp_loadvm(Monitor *mon, const QDict *qdict);
> +void hmp_savevm(Monitor *mon, const QDict *qdict);
> void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
> void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
> void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index b6daf9d..914c36c 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -75,7 +75,6 @@ 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);
>
> -void hmp_savevm(Monitor *mon, const QDict *qdict);
> int save_vmstate(const char *name);
> int load_vmstate(const char *name);
> void hmp_delvm(Monitor *mon, const QDict *qdict);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index ff934aa..bbff4d8 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2171,11 +2171,6 @@ int save_vmstate(const char *name)
> return ret;
> }
>
> -void hmp_savevm(Monitor *mon, const QDict *qdict)
> -{
> - save_vmstate(qdict_get_try_str(qdict, "name"));
> -}
> -
> void qmp_xen_save_devices_state(const char *filename, Error **errp)
> {
> QEMUFile *f;
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 4/6] monitor: Move hmp_delvm from savevm.c to hmp.c
2017-04-25 10:24 [Qemu-devel] [PATCH 0/6] Move snapshots commands to hmp Juan Quintela
` (2 preceding siblings ...)
2017-04-25 10:24 ` [Qemu-devel] [PATCH 3/6] monitor: Move hmp_savevm from savevm.c " Juan Quintela
@ 2017-04-25 10:24 ` Juan Quintela
2017-04-25 13:34 ` Laurent Vivier
2017-04-25 10:24 ` [Qemu-devel] [PATCH 5/6] monitor: Move hmp_info_snapshots " Juan Quintela
` (2 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Juan Quintela @ 2017-04-25 10:24 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx
It really uses block/* stuff, not migration one.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
hmp.c | 13 +++++++++++++
hmp.h | 1 +
include/sysemu/sysemu.h | 1 -
migration/savevm.c | 13 -------------
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/hmp.c b/hmp.c
index a82a952..bb739ce 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1286,6 +1286,19 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
save_vmstate(qdict_get_try_str(qdict, "name"));
}
+void hmp_delvm(Monitor *mon, const QDict *qdict)
+{
+ BlockDriverState *bs;
+ Error *err;
+ const char *name = qdict_get_str(qdict, "name");
+
+ if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
+ error_reportf_err(err,
+ "Error while deleting snapshot on device '%s': ",
+ bdrv_get_device_name(bs));
+ }
+}
+
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
{
qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index b302c8d..6a402b1 100644
--- a/hmp.h
+++ b/hmp.h
@@ -65,6 +65,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
void hmp_drive_backup(Monitor *mon, const QDict *qdict);
void hmp_loadvm(Monitor *mon, const QDict *qdict);
void hmp_savevm(Monitor *mon, const QDict *qdict);
+void hmp_delvm(Monitor *mon, const QDict *qdict);
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 914c36c..e4f355ceb 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -77,7 +77,6 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify);
int save_vmstate(const char *name);
int load_vmstate(const char *name);
-void hmp_delvm(Monitor *mon, const QDict *qdict);
void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
void qemu_announce_self(void);
diff --git a/migration/savevm.c b/migration/savevm.c
index bbff4d8..acd304b 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2304,19 +2304,6 @@ int load_vmstate(const char *name)
return 0;
}
-void hmp_delvm(Monitor *mon, const QDict *qdict)
-{
- BlockDriverState *bs;
- Error *err;
- const char *name = qdict_get_str(qdict, "name");
-
- if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
- error_reportf_err(err,
- "Error while deleting snapshot on device '%s': ",
- bdrv_get_device_name(bs));
- }
-}
-
void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
{
BlockDriverState *bs, *bs1;
--
2.9.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 4/6] monitor: Move hmp_delvm from savevm.c to hmp.c
2017-04-25 10:24 ` [Qemu-devel] [PATCH 4/6] monitor: Move hmp_delvm " Juan Quintela
@ 2017-04-25 13:34 ` Laurent Vivier
0 siblings, 0 replies; 22+ messages in thread
From: Laurent Vivier @ 2017-04-25 13:34 UTC (permalink / raw)
To: Juan Quintela, qemu-devel; +Cc: dgilbert, peterx
On 25/04/2017 12:24, Juan Quintela wrote:
> It really uses block/* stuff, not migration one.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> ---
> hmp.c | 13 +++++++++++++
> hmp.h | 1 +
> include/sysemu/sysemu.h | 1 -
> migration/savevm.c | 13 -------------
> 4 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index a82a952..bb739ce 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1286,6 +1286,19 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
> save_vmstate(qdict_get_try_str(qdict, "name"));
> }
>
> +void hmp_delvm(Monitor *mon, const QDict *qdict)
> +{
> + BlockDriverState *bs;
> + Error *err;
> + const char *name = qdict_get_str(qdict, "name");
> +
> + if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
> + error_reportf_err(err,
> + "Error while deleting snapshot on device '%s': ",
> + bdrv_get_device_name(bs));
> + }
> +}
> +
> void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
> {
> qmp_migrate_cancel(NULL);
> diff --git a/hmp.h b/hmp.h
> index b302c8d..6a402b1 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -65,6 +65,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
> void hmp_drive_backup(Monitor *mon, const QDict *qdict);
> void hmp_loadvm(Monitor *mon, const QDict *qdict);
> void hmp_savevm(Monitor *mon, const QDict *qdict);
> +void hmp_delvm(Monitor *mon, const QDict *qdict);
> void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
> void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
> void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 914c36c..e4f355ceb 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -77,7 +77,6 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify);
>
> int save_vmstate(const char *name);
> int load_vmstate(const char *name);
> -void hmp_delvm(Monitor *mon, const QDict *qdict);
> void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
>
> void qemu_announce_self(void);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index bbff4d8..acd304b 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2304,19 +2304,6 @@ int load_vmstate(const char *name)
> return 0;
> }
>
> -void hmp_delvm(Monitor *mon, const QDict *qdict)
> -{
> - BlockDriverState *bs;
> - Error *err;
> - const char *name = qdict_get_str(qdict, "name");
> -
> - if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
> - error_reportf_err(err,
> - "Error while deleting snapshot on device '%s': ",
> - bdrv_get_device_name(bs));
> - }
> -}
> -
> void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
> {
> BlockDriverState *bs, *bs1;
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 5/6] monitor: Move hmp_info_snapshots from savevm.c to hmp.c
2017-04-25 10:24 [Qemu-devel] [PATCH 0/6] Move snapshots commands to hmp Juan Quintela
` (3 preceding siblings ...)
2017-04-25 10:24 ` [Qemu-devel] [PATCH 4/6] monitor: Move hmp_delvm " Juan Quintela
@ 2017-04-25 10:24 ` Juan Quintela
2017-04-25 14:15 ` Laurent Vivier
2017-04-25 10:24 ` [Qemu-devel] [PATCH 6/6] migration: Pass Error ** argument to {save, load}_vmstate Juan Quintela
2017-04-28 15:07 ` [Qemu-devel] [PATCH 0/6] Move snapshots commands to hmp Dr. David Alan Gilbert
6 siblings, 1 reply; 22+ messages in thread
From: Juan Quintela @ 2017-04-25 10:24 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx
It only uses block/* functions, nothing from migration.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
hmp.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++
hmp.h | 1 +
include/sysemu/sysemu.h | 1 -
migration/savevm.c | 147 ------------------------------------------------
4 files changed, 144 insertions(+), 148 deletions(-)
diff --git a/hmp.c b/hmp.c
index bb739ce..bd7b1ca 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1299,6 +1299,149 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
}
}
+void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
+{
+ BlockDriverState *bs, *bs1;
+ BdrvNextIterator it1;
+ QEMUSnapshotInfo *sn_tab, *sn;
+ bool no_snapshot = true;
+ int nb_sns, i;
+ int total;
+ int *global_snapshots;
+ AioContext *aio_context;
+
+ typedef struct SnapshotEntry {
+ QEMUSnapshotInfo sn;
+ QTAILQ_ENTRY(SnapshotEntry) next;
+ } SnapshotEntry;
+
+ typedef struct ImageEntry {
+ const char *imagename;
+ QTAILQ_ENTRY(ImageEntry) next;
+ QTAILQ_HEAD(, SnapshotEntry) snapshots;
+ } ImageEntry;
+
+ QTAILQ_HEAD(, ImageEntry) image_list =
+ QTAILQ_HEAD_INITIALIZER(image_list);
+
+ ImageEntry *image_entry, *next_ie;
+ SnapshotEntry *snapshot_entry;
+
+ bs = bdrv_all_find_vmstate_bs();
+ if (!bs) {
+ monitor_printf(mon, "No available block device supports snapshots\n");
+ return;
+ }
+ aio_context = bdrv_get_aio_context(bs);
+
+ aio_context_acquire(aio_context);
+ nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+ aio_context_release(aio_context);
+
+ if (nb_sns < 0) {
+ monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
+ return;
+ }
+
+ for (bs1 = bdrv_first(&it1); bs1; bs1 = bdrv_next(&it1)) {
+ int bs1_nb_sns = 0;
+ ImageEntry *ie;
+ SnapshotEntry *se;
+ AioContext *ctx = bdrv_get_aio_context(bs1);
+
+ aio_context_acquire(ctx);
+ if (bdrv_can_snapshot(bs1)) {
+ sn = NULL;
+ bs1_nb_sns = bdrv_snapshot_list(bs1, &sn);
+ if (bs1_nb_sns > 0) {
+ no_snapshot = false;
+ ie = g_new0(ImageEntry, 1);
+ ie->imagename = bdrv_get_device_name(bs1);
+ QTAILQ_INIT(&ie->snapshots);
+ QTAILQ_INSERT_TAIL(&image_list, ie, next);
+ for (i = 0; i < bs1_nb_sns; i++) {
+ se = g_new0(SnapshotEntry, 1);
+ se->sn = sn[i];
+ QTAILQ_INSERT_TAIL(&ie->snapshots, se, next);
+ }
+ }
+ g_free(sn);
+ }
+ aio_context_release(ctx);
+ }
+
+ if (no_snapshot) {
+ monitor_printf(mon, "There is no snapshot available.\n");
+ return;
+ }
+
+ global_snapshots = g_new0(int, nb_sns);
+ total = 0;
+ for (i = 0; i < nb_sns; i++) {
+ SnapshotEntry *next_sn;
+ if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) {
+ global_snapshots[total] = i;
+ total++;
+ QTAILQ_FOREACH(image_entry, &image_list, next) {
+ QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snapshots,
+ next, next_sn) {
+ if (!strcmp(sn_tab[i].name, snapshot_entry->sn.name)) {
+ QTAILQ_REMOVE(&image_entry->snapshots, snapshot_entry,
+ next);
+ g_free(snapshot_entry);
+ }
+ }
+ }
+ }
+ }
+
+ monitor_printf(mon, "List of snapshots present on all disks:\n");
+
+ if (total > 0) {
+ bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL);
+ monitor_printf(mon, "\n");
+ for (i = 0; i < total; i++) {
+ sn = &sn_tab[global_snapshots[i]];
+ /* The ID is not guaranteed to be the same on all images, so
+ * overwrite it.
+ */
+ pstrcpy(sn->id_str, sizeof(sn->id_str), "--");
+ bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn);
+ monitor_printf(mon, "\n");
+ }
+ } else {
+ monitor_printf(mon, "None\n");
+ }
+
+ QTAILQ_FOREACH(image_entry, &image_list, next) {
+ if (QTAILQ_EMPTY(&image_entry->snapshots)) {
+ continue;
+ }
+ monitor_printf(mon,
+ "\nList of partial (non-loadable) snapshots on '%s':\n",
+ image_entry->imagename);
+ bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL);
+ monitor_printf(mon, "\n");
+ QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) {
+ bdrv_snapshot_dump((fprintf_function)monitor_printf, mon,
+ &snapshot_entry->sn);
+ monitor_printf(mon, "\n");
+ }
+ }
+
+ QTAILQ_FOREACH_SAFE(image_entry, &image_list, next, next_ie) {
+ SnapshotEntry *next_sn;
+ QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snapshots, next,
+ next_sn) {
+ g_free(snapshot_entry);
+ }
+ g_free(image_entry);
+ }
+ g_free(sn_tab);
+ g_free(global_snapshots);
+
+}
+
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
{
qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index 6a402b1..37bb65a 100644
--- a/hmp.h
+++ b/hmp.h
@@ -66,6 +66,7 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict);
void hmp_loadvm(Monitor *mon, const QDict *qdict);
void hmp_savevm(Monitor *mon, const QDict *qdict);
void hmp_delvm(Monitor *mon, const QDict *qdict);
+void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index e4f355ceb..15656b7 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -77,7 +77,6 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify);
int save_vmstate(const char *name);
int load_vmstate(const char *name);
-void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
void qemu_announce_self(void);
diff --git a/migration/savevm.c b/migration/savevm.c
index acd304b..8dd4306 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -33,15 +33,12 @@
#include "hw/qdev.h"
#include "hw/xen/xen.h"
#include "net/net.h"
-#include "monitor/monitor.h"
#include "sysemu/sysemu.h"
#include "qemu/timer.h"
-#include "audio/audio.h"
#include "migration/migration.h"
#include "migration/postcopy-ram.h"
#include "qapi/qmp/qerror.h"
#include "qemu/error-report.h"
-#include "qemu/sockets.h"
#include "qemu/queue.h"
#include "sysemu/cpus.h"
#include "exec/memory.h"
@@ -50,7 +47,6 @@
#include "qemu/bitops.h"
#include "qemu/iov.h"
#include "block/snapshot.h"
-#include "block/qapi.h"
#include "qemu/cutils.h"
#include "io/channel-buffer.h"
#include "io/channel-file.h"
@@ -2304,149 +2300,6 @@ int load_vmstate(const char *name)
return 0;
}
-void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
-{
- BlockDriverState *bs, *bs1;
- BdrvNextIterator it1;
- QEMUSnapshotInfo *sn_tab, *sn;
- bool no_snapshot = true;
- int nb_sns, i;
- int total;
- int *global_snapshots;
- AioContext *aio_context;
-
- typedef struct SnapshotEntry {
- QEMUSnapshotInfo sn;
- QTAILQ_ENTRY(SnapshotEntry) next;
- } SnapshotEntry;
-
- typedef struct ImageEntry {
- const char *imagename;
- QTAILQ_ENTRY(ImageEntry) next;
- QTAILQ_HEAD(, SnapshotEntry) snapshots;
- } ImageEntry;
-
- QTAILQ_HEAD(, ImageEntry) image_list =
- QTAILQ_HEAD_INITIALIZER(image_list);
-
- ImageEntry *image_entry, *next_ie;
- SnapshotEntry *snapshot_entry;
-
- bs = bdrv_all_find_vmstate_bs();
- if (!bs) {
- monitor_printf(mon, "No available block device supports snapshots\n");
- return;
- }
- aio_context = bdrv_get_aio_context(bs);
-
- aio_context_acquire(aio_context);
- nb_sns = bdrv_snapshot_list(bs, &sn_tab);
- aio_context_release(aio_context);
-
- if (nb_sns < 0) {
- monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
- return;
- }
-
- for (bs1 = bdrv_first(&it1); bs1; bs1 = bdrv_next(&it1)) {
- int bs1_nb_sns = 0;
- ImageEntry *ie;
- SnapshotEntry *se;
- AioContext *ctx = bdrv_get_aio_context(bs1);
-
- aio_context_acquire(ctx);
- if (bdrv_can_snapshot(bs1)) {
- sn = NULL;
- bs1_nb_sns = bdrv_snapshot_list(bs1, &sn);
- if (bs1_nb_sns > 0) {
- no_snapshot = false;
- ie = g_new0(ImageEntry, 1);
- ie->imagename = bdrv_get_device_name(bs1);
- QTAILQ_INIT(&ie->snapshots);
- QTAILQ_INSERT_TAIL(&image_list, ie, next);
- for (i = 0; i < bs1_nb_sns; i++) {
- se = g_new0(SnapshotEntry, 1);
- se->sn = sn[i];
- QTAILQ_INSERT_TAIL(&ie->snapshots, se, next);
- }
- }
- g_free(sn);
- }
- aio_context_release(ctx);
- }
-
- if (no_snapshot) {
- monitor_printf(mon, "There is no snapshot available.\n");
- return;
- }
-
- global_snapshots = g_new0(int, nb_sns);
- total = 0;
- for (i = 0; i < nb_sns; i++) {
- SnapshotEntry *next_sn;
- if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) {
- global_snapshots[total] = i;
- total++;
- QTAILQ_FOREACH(image_entry, &image_list, next) {
- QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snapshots,
- next, next_sn) {
- if (!strcmp(sn_tab[i].name, snapshot_entry->sn.name)) {
- QTAILQ_REMOVE(&image_entry->snapshots, snapshot_entry,
- next);
- g_free(snapshot_entry);
- }
- }
- }
- }
- }
-
- monitor_printf(mon, "List of snapshots present on all disks:\n");
-
- if (total > 0) {
- bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL);
- monitor_printf(mon, "\n");
- for (i = 0; i < total; i++) {
- sn = &sn_tab[global_snapshots[i]];
- /* The ID is not guaranteed to be the same on all images, so
- * overwrite it.
- */
- pstrcpy(sn->id_str, sizeof(sn->id_str), "--");
- bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn);
- monitor_printf(mon, "\n");
- }
- } else {
- monitor_printf(mon, "None\n");
- }
-
- QTAILQ_FOREACH(image_entry, &image_list, next) {
- if (QTAILQ_EMPTY(&image_entry->snapshots)) {
- continue;
- }
- monitor_printf(mon,
- "\nList of partial (non-loadable) snapshots on '%s':\n",
- image_entry->imagename);
- bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL);
- monitor_printf(mon, "\n");
- QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) {
- bdrv_snapshot_dump((fprintf_function)monitor_printf, mon,
- &snapshot_entry->sn);
- monitor_printf(mon, "\n");
- }
- }
-
- QTAILQ_FOREACH_SAFE(image_entry, &image_list, next, next_ie) {
- SnapshotEntry *next_sn;
- QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snapshots, next,
- next_sn) {
- g_free(snapshot_entry);
- }
- g_free(image_entry);
- }
- g_free(sn_tab);
- g_free(global_snapshots);
-
-}
-
void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
{
qemu_ram_set_idstr(mr->ram_block,
--
2.9.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] monitor: Move hmp_info_snapshots from savevm.c to hmp.c
2017-04-25 10:24 ` [Qemu-devel] [PATCH 5/6] monitor: Move hmp_info_snapshots " Juan Quintela
@ 2017-04-25 14:15 ` Laurent Vivier
2017-04-25 16:48 ` Juan Quintela
0 siblings, 1 reply; 22+ messages in thread
From: Laurent Vivier @ 2017-04-25 14:15 UTC (permalink / raw)
To: Juan Quintela, qemu-devel; +Cc: dgilbert, peterx
On 25/04/2017 12:24, Juan Quintela wrote:
> It only uses block/* functions, nothing from migration.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Perhaps the monitor.h include in savevm.c can be removed in PATCH 1/6?
Laurent
> ---
> hmp.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++
> hmp.h | 1 +
> include/sysemu/sysemu.h | 1 -
> migration/savevm.c | 147 ------------------------------------------------
> 4 files changed, 144 insertions(+), 148 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index bb739ce..bd7b1ca 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1299,6 +1299,149 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
> }
> }
>
> +void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
> +{
> + BlockDriverState *bs, *bs1;
> + BdrvNextIterator it1;
> + QEMUSnapshotInfo *sn_tab, *sn;
> + bool no_snapshot = true;
> + int nb_sns, i;
> + int total;
> + int *global_snapshots;
> + AioContext *aio_context;
> +
> + typedef struct SnapshotEntry {
> + QEMUSnapshotInfo sn;
> + QTAILQ_ENTRY(SnapshotEntry) next;
> + } SnapshotEntry;
> +
> + typedef struct ImageEntry {
> + const char *imagename;
> + QTAILQ_ENTRY(ImageEntry) next;
> + QTAILQ_HEAD(, SnapshotEntry) snapshots;
> + } ImageEntry;
> +
> + QTAILQ_HEAD(, ImageEntry) image_list =
> + QTAILQ_HEAD_INITIALIZER(image_list);
> +
> + ImageEntry *image_entry, *next_ie;
> + SnapshotEntry *snapshot_entry;
> +
> + bs = bdrv_all_find_vmstate_bs();
> + if (!bs) {
> + monitor_printf(mon, "No available block device supports snapshots\n");
> + return;
> + }
> + aio_context = bdrv_get_aio_context(bs);
> +
> + aio_context_acquire(aio_context);
> + nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> + aio_context_release(aio_context);
> +
> + if (nb_sns < 0) {
> + monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
> + return;
> + }
> +
> + for (bs1 = bdrv_first(&it1); bs1; bs1 = bdrv_next(&it1)) {
> + int bs1_nb_sns = 0;
> + ImageEntry *ie;
> + SnapshotEntry *se;
> + AioContext *ctx = bdrv_get_aio_context(bs1);
> +
> + aio_context_acquire(ctx);
> + if (bdrv_can_snapshot(bs1)) {
> + sn = NULL;
> + bs1_nb_sns = bdrv_snapshot_list(bs1, &sn);
> + if (bs1_nb_sns > 0) {
> + no_snapshot = false;
> + ie = g_new0(ImageEntry, 1);
> + ie->imagename = bdrv_get_device_name(bs1);
> + QTAILQ_INIT(&ie->snapshots);
> + QTAILQ_INSERT_TAIL(&image_list, ie, next);
> + for (i = 0; i < bs1_nb_sns; i++) {
> + se = g_new0(SnapshotEntry, 1);
> + se->sn = sn[i];
> + QTAILQ_INSERT_TAIL(&ie->snapshots, se, next);
> + }
> + }
> + g_free(sn);
> + }
> + aio_context_release(ctx);
> + }
> +
> + if (no_snapshot) {
> + monitor_printf(mon, "There is no snapshot available.\n");
> + return;
> + }
> +
> + global_snapshots = g_new0(int, nb_sns);
> + total = 0;
> + for (i = 0; i < nb_sns; i++) {
> + SnapshotEntry *next_sn;
> + if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) {
> + global_snapshots[total] = i;
> + total++;
> + QTAILQ_FOREACH(image_entry, &image_list, next) {
> + QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snapshots,
> + next, next_sn) {
> + if (!strcmp(sn_tab[i].name, snapshot_entry->sn.name)) {
> + QTAILQ_REMOVE(&image_entry->snapshots, snapshot_entry,
> + next);
> + g_free(snapshot_entry);
> + }
> + }
> + }
> + }
> + }
> +
> + monitor_printf(mon, "List of snapshots present on all disks:\n");
> +
> + if (total > 0) {
> + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL);
> + monitor_printf(mon, "\n");
> + for (i = 0; i < total; i++) {
> + sn = &sn_tab[global_snapshots[i]];
> + /* The ID is not guaranteed to be the same on all images, so
> + * overwrite it.
> + */
> + pstrcpy(sn->id_str, sizeof(sn->id_str), "--");
> + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn);
> + monitor_printf(mon, "\n");
> + }
> + } else {
> + monitor_printf(mon, "None\n");
> + }
> +
> + QTAILQ_FOREACH(image_entry, &image_list, next) {
> + if (QTAILQ_EMPTY(&image_entry->snapshots)) {
> + continue;
> + }
> + monitor_printf(mon,
> + "\nList of partial (non-loadable) snapshots on '%s':\n",
> + image_entry->imagename);
> + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL);
> + monitor_printf(mon, "\n");
> + QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) {
> + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon,
> + &snapshot_entry->sn);
> + monitor_printf(mon, "\n");
> + }
> + }
> +
> + QTAILQ_FOREACH_SAFE(image_entry, &image_list, next, next_ie) {
> + SnapshotEntry *next_sn;
> + QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snapshots, next,
> + next_sn) {
> + g_free(snapshot_entry);
> + }
> + g_free(image_entry);
> + }
> + g_free(sn_tab);
> + g_free(global_snapshots);
> +
> +}
> +
> void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
> {
> qmp_migrate_cancel(NULL);
> diff --git a/hmp.h b/hmp.h
> index 6a402b1..37bb65a 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -66,6 +66,7 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict);
> void hmp_loadvm(Monitor *mon, const QDict *qdict);
> void hmp_savevm(Monitor *mon, const QDict *qdict);
> void hmp_delvm(Monitor *mon, const QDict *qdict);
> +void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
> void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
> void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
> void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index e4f355ceb..15656b7 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -77,7 +77,6 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify);
>
> int save_vmstate(const char *name);
> int load_vmstate(const char *name);
> -void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
>
> void qemu_announce_self(void);
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index acd304b..8dd4306 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -33,15 +33,12 @@
> #include "hw/qdev.h"
> #include "hw/xen/xen.h"
> #include "net/net.h"
> -#include "monitor/monitor.h"
> #include "sysemu/sysemu.h"
> #include "qemu/timer.h"
> -#include "audio/audio.h"
> #include "migration/migration.h"
> #include "migration/postcopy-ram.h"
> #include "qapi/qmp/qerror.h"
> #include "qemu/error-report.h"
> -#include "qemu/sockets.h"
> #include "qemu/queue.h"
> #include "sysemu/cpus.h"
> #include "exec/memory.h"
> @@ -50,7 +47,6 @@
> #include "qemu/bitops.h"
> #include "qemu/iov.h"
> #include "block/snapshot.h"
> -#include "block/qapi.h"
> #include "qemu/cutils.h"
> #include "io/channel-buffer.h"
> #include "io/channel-file.h"
> @@ -2304,149 +2300,6 @@ int load_vmstate(const char *name)
> return 0;
> }
>
> -void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
> -{
> - BlockDriverState *bs, *bs1;
> - BdrvNextIterator it1;
> - QEMUSnapshotInfo *sn_tab, *sn;
> - bool no_snapshot = true;
> - int nb_sns, i;
> - int total;
> - int *global_snapshots;
> - AioContext *aio_context;
> -
> - typedef struct SnapshotEntry {
> - QEMUSnapshotInfo sn;
> - QTAILQ_ENTRY(SnapshotEntry) next;
> - } SnapshotEntry;
> -
> - typedef struct ImageEntry {
> - const char *imagename;
> - QTAILQ_ENTRY(ImageEntry) next;
> - QTAILQ_HEAD(, SnapshotEntry) snapshots;
> - } ImageEntry;
> -
> - QTAILQ_HEAD(, ImageEntry) image_list =
> - QTAILQ_HEAD_INITIALIZER(image_list);
> -
> - ImageEntry *image_entry, *next_ie;
> - SnapshotEntry *snapshot_entry;
> -
> - bs = bdrv_all_find_vmstate_bs();
> - if (!bs) {
> - monitor_printf(mon, "No available block device supports snapshots\n");
> - return;
> - }
> - aio_context = bdrv_get_aio_context(bs);
> -
> - aio_context_acquire(aio_context);
> - nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> - aio_context_release(aio_context);
> -
> - if (nb_sns < 0) {
> - monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
> - return;
> - }
> -
> - for (bs1 = bdrv_first(&it1); bs1; bs1 = bdrv_next(&it1)) {
> - int bs1_nb_sns = 0;
> - ImageEntry *ie;
> - SnapshotEntry *se;
> - AioContext *ctx = bdrv_get_aio_context(bs1);
> -
> - aio_context_acquire(ctx);
> - if (bdrv_can_snapshot(bs1)) {
> - sn = NULL;
> - bs1_nb_sns = bdrv_snapshot_list(bs1, &sn);
> - if (bs1_nb_sns > 0) {
> - no_snapshot = false;
> - ie = g_new0(ImageEntry, 1);
> - ie->imagename = bdrv_get_device_name(bs1);
> - QTAILQ_INIT(&ie->snapshots);
> - QTAILQ_INSERT_TAIL(&image_list, ie, next);
> - for (i = 0; i < bs1_nb_sns; i++) {
> - se = g_new0(SnapshotEntry, 1);
> - se->sn = sn[i];
> - QTAILQ_INSERT_TAIL(&ie->snapshots, se, next);
> - }
> - }
> - g_free(sn);
> - }
> - aio_context_release(ctx);
> - }
> -
> - if (no_snapshot) {
> - monitor_printf(mon, "There is no snapshot available.\n");
> - return;
> - }
> -
> - global_snapshots = g_new0(int, nb_sns);
> - total = 0;
> - for (i = 0; i < nb_sns; i++) {
> - SnapshotEntry *next_sn;
> - if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) {
> - global_snapshots[total] = i;
> - total++;
> - QTAILQ_FOREACH(image_entry, &image_list, next) {
> - QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snapshots,
> - next, next_sn) {
> - if (!strcmp(sn_tab[i].name, snapshot_entry->sn.name)) {
> - QTAILQ_REMOVE(&image_entry->snapshots, snapshot_entry,
> - next);
> - g_free(snapshot_entry);
> - }
> - }
> - }
> - }
> - }
> -
> - monitor_printf(mon, "List of snapshots present on all disks:\n");
> -
> - if (total > 0) {
> - bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL);
> - monitor_printf(mon, "\n");
> - for (i = 0; i < total; i++) {
> - sn = &sn_tab[global_snapshots[i]];
> - /* The ID is not guaranteed to be the same on all images, so
> - * overwrite it.
> - */
> - pstrcpy(sn->id_str, sizeof(sn->id_str), "--");
> - bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn);
> - monitor_printf(mon, "\n");
> - }
> - } else {
> - monitor_printf(mon, "None\n");
> - }
> -
> - QTAILQ_FOREACH(image_entry, &image_list, next) {
> - if (QTAILQ_EMPTY(&image_entry->snapshots)) {
> - continue;
> - }
> - monitor_printf(mon,
> - "\nList of partial (non-loadable) snapshots on '%s':\n",
> - image_entry->imagename);
> - bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL);
> - monitor_printf(mon, "\n");
> - QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) {
> - bdrv_snapshot_dump((fprintf_function)monitor_printf, mon,
> - &snapshot_entry->sn);
> - monitor_printf(mon, "\n");
> - }
> - }
> -
> - QTAILQ_FOREACH_SAFE(image_entry, &image_list, next, next_ie) {
> - SnapshotEntry *next_sn;
> - QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snapshots, next,
> - next_sn) {
> - g_free(snapshot_entry);
> - }
> - g_free(image_entry);
> - }
> - g_free(sn_tab);
> - g_free(global_snapshots);
> -
> -}
> -
> void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
> {
> qemu_ram_set_idstr(mr->ram_block,
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] monitor: Move hmp_info_snapshots from savevm.c to hmp.c
2017-04-25 14:15 ` Laurent Vivier
@ 2017-04-25 16:48 ` Juan Quintela
0 siblings, 0 replies; 22+ messages in thread
From: Juan Quintela @ 2017-04-25 16:48 UTC (permalink / raw)
To: Laurent Vivier; +Cc: qemu-devel, dgilbert, peterx
Laurent Vivier <lvivier@redhat.com> wrote:
> On 25/04/2017 12:24, Juan Quintela wrote:
>> It only uses block/* functions, nothing from migration.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
>
> Perhaps the monitor.h include in savevm.c can be removed in PATCH 1/6?
hmp_info_snapshots still uses monitor_printf(), so I have to left it
until here.
Thanks, Juan.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 6/6] migration: Pass Error ** argument to {save, load}_vmstate
2017-04-25 10:24 [Qemu-devel] [PATCH 0/6] Move snapshots commands to hmp Juan Quintela
` (4 preceding siblings ...)
2017-04-25 10:24 ` [Qemu-devel] [PATCH 5/6] monitor: Move hmp_info_snapshots " Juan Quintela
@ 2017-04-25 10:24 ` Juan Quintela
2017-04-25 15:21 ` Laurent Vivier
2017-04-28 14:47 ` Dr. David Alan Gilbert
2017-04-28 15:07 ` [Qemu-devel] [PATCH 0/6] Move snapshots commands to hmp Dr. David Alan Gilbert
6 siblings, 2 replies; 22+ messages in thread
From: Juan Quintela @ 2017-04-25 10:24 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] migration: Pass Error ** argument to {save, load}_vmstate
2017-04-25 10:24 ` [Qemu-devel] [PATCH 6/6] migration: Pass Error ** argument to {save, load}_vmstate Juan Quintela
@ 2017-04-25 15:21 ` Laurent Vivier
2017-04-25 17:00 ` Juan Quintela
2017-04-28 14:47 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 22+ messages in thread
From: Laurent Vivier @ 2017-04-25 15:21 UTC (permalink / raw)
To: Juan Quintela, qemu-devel; +Cc: dgilbert, peterx
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] migration: Pass Error ** argument to {save, load}_vmstate
2017-04-25 15:21 ` Laurent Vivier
@ 2017-04-25 17:00 ` Juan Quintela
2017-04-25 17:10 ` Laurent Vivier
0 siblings, 1 reply; 22+ messages in thread
From: Juan Quintela @ 2017-04-25 17:00 UTC (permalink / raw)
To: Laurent Vivier; +Cc: qemu-devel, dgilbert, peterx
Laurent Vivier <lvivier@redhat.com> wrote:
> 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?
Thanks.
>> {
>> + 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.
I was very happy with your suggestion. But then I realized that if I
use error_fatal, I would lost the error_report() messages, right?
Later, Juan.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] migration: Pass Error ** argument to {save, load}_vmstate
2017-04-25 17:00 ` Juan Quintela
@ 2017-04-25 17:10 ` Laurent Vivier
2017-04-25 17:31 ` Juan Quintela
0 siblings, 1 reply; 22+ messages in thread
From: Laurent Vivier @ 2017-04-25 17:10 UTC (permalink / raw)
To: quintela; +Cc: qemu-devel, dgilbert, peterx
On 25/04/2017 19:00, Juan Quintela wrote:
> Laurent Vivier <lvivier@redhat.com> wrote:
>> 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?
>
> Thanks.
>>> {
>>> + 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.
>
> I was very happy with your suggestion. But then I realized that if I
> use error_fatal, I would lost the error_report() messages, right?
The "Could not create snapshot for icount record" and "Could not load
snapshot for icount replay", yes.
But you keep the one from the inside load_vmstate().
Isn't it enough?
Laurent
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] migration: Pass Error ** argument to {save, load}_vmstate
2017-04-25 17:10 ` Laurent Vivier
@ 2017-04-25 17:31 ` Juan Quintela
0 siblings, 0 replies; 22+ messages in thread
From: Juan Quintela @ 2017-04-25 17:31 UTC (permalink / raw)
To: Laurent Vivier; +Cc: qemu-devel, dgilbert, peterx
Laurent Vivier <lvivier@redhat.com> wrote:
> On 25/04/2017 19:00, Juan Quintela wrote:
>> Laurent Vivier <lvivier@redhat.com> wrote:
>>> On 25/04/2017 12:24, Juan Quintela wrote:
>>>> {
>>>> + 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.
>>
>> I was very happy with your suggestion. But then I realized that if I
>> use error_fatal, I would lost the error_report() messages, right?
>
> The "Could not create snapshot for icount record" and "Could not load
> snapshot for icount replay", yes.
>
> But you keep the one from the inside load_vmstate().
if (!bdrv_all_can_snapshot(&bs)) {
error_setg(errp, "Device '%s' is writable but does not support "
"snapshots", bdrv_get_device_name(bs));
return ret;
}
This is the 1st error on save_vmstate. My understanding is that now I
get something like (two messages):
Device 'foo' is writable but does not support snapshots
Could not create snapshot for icount record
If I change to error_fatal, only the 1st message will appear. And I am
not replay maintainer to decide the change O:-)
> Isn't it enough?
Dunno.
Later, Juan.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] migration: Pass Error ** argument to {save, load}_vmstate
2017-04-25 10:24 ` [Qemu-devel] [PATCH 6/6] migration: Pass Error ** argument to {save, load}_vmstate Juan Quintela
2017-04-25 15:21 ` Laurent Vivier
@ 2017-04-28 14:47 ` Dr. David Alan Gilbert
2017-04-28 15:19 ` Markus Armbruster
2017-04-28 15:20 ` Eric Blake
1 sibling, 2 replies; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2017-04-28 14:47 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, lvivier, peterx
* 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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] migration: Pass Error ** argument to {save, load}_vmstate
2017-04-28 14:47 ` Dr. David Alan Gilbert
@ 2017-04-28 15:19 ` Markus Armbruster
2017-04-28 16:13 ` Dr. David Alan Gilbert
2017-04-28 15:20 ` Eric Blake
1 sibling, 1 reply; 22+ messages in thread
From: Markus Armbruster @ 2017-04-28 15:19 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Juan Quintela, lvivier, qemu-devel, peterx
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> * 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)
Quote qapi/error.h:
* Receive an error and pass it on to the caller:
* Error *err = NULL;
* foo(arg, &err);
* if (err) {
* handle the error...
* error_propagate(errp, err);
* }
* where Error **errp is a parameter, by convention the last one.
*
* Do *not* "optimize" this to
* foo(arg, errp);
* if (*errp) { // WRONG!
* handle the error...
* }
* because errp may be NULL!
*
* But when all you do with the error is pass it on, please use
* foo(arg, errp);
* for readability.
Neglects to mention you can error_prepend() and still avoid
error_propagate(), but the comment is long enough as it is.
>> 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).
I guess you mean &error_fatal.
Consider error_report_err().
>
> 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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] migration: Pass Error ** argument to {save, load}_vmstate
2017-04-28 15:19 ` Markus Armbruster
@ 2017-04-28 16:13 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2017-04-28 16:13 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Juan Quintela, lvivier, qemu-devel, peterx
* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>
> > * 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)
>
> Quote qapi/error.h:
>
> * Receive an error and pass it on to the caller:
> * Error *err = NULL;
> * foo(arg, &err);
> * if (err) {
> * handle the error...
> * error_propagate(errp, err);
> * }
> * where Error **errp is a parameter, by convention the last one.
> *
> * Do *not* "optimize" this to
> * foo(arg, errp);
> * if (*errp) { // WRONG!
> * handle the error...
> * }
> * because errp may be NULL!
> *
> * But when all you do with the error is pass it on, please use
> * foo(arg, errp);
> * for readability.
>
> Neglects to mention you can error_prepend() and still avoid
> error_propagate(), but the comment is long enough as it is.
Ah ok, that does make life easier.
> >> 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).
>
> I guess you mean &error_fatal.
>
> Consider error_report_err().
Yes, thanks.
Dave
> >
> > 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
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] migration: Pass Error ** argument to {save, load}_vmstate
2017-04-28 14:47 ` Dr. David Alan Gilbert
2017-04-28 15:19 ` Markus Armbruster
@ 2017-04-28 15:20 ` Eric Blake
1 sibling, 0 replies; 22+ messages in thread
From: Eric Blake @ 2017-04-28 15:20 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Juan Quintela; +Cc: lvivier, qemu-devel, peterx
[-- Attachment #1: Type: text/plain, Size: 1978 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 0/6] Move snapshots commands to hmp
2017-04-25 10:24 [Qemu-devel] [PATCH 0/6] Move snapshots commands to hmp Juan Quintela
` (5 preceding siblings ...)
2017-04-25 10:24 ` [Qemu-devel] [PATCH 6/6] migration: Pass Error ** argument to {save, load}_vmstate Juan Quintela
@ 2017-04-28 15:07 ` Dr. David Alan Gilbert
6 siblings, 0 replies; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2017-04-28 15:07 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, lvivier, peterx
* Juan Quintela (quintela@redhat.com) wrote:
> Hi
>
> This series:
> - Move snapshots commands to hmp.c, as they don't have code for migration
> - Make them work with errors in a modern way instead of writting to the monitor
> - make paolo happy and use hmp_handle_error
>
> Later, Juan.
I'm happy with 1-5, 6 though, see comments.
If you want I can merge 1-5 via my next HMP pull, otherwise
if you want to do them via migration I agree with Laurent's Rb's so
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> Juan Quintela (6):
> monitor: Remove monitor parameter from save_vmstate
> monitor: Move hmp_loadvm from monitor.c to hmp.c
> monitor: Move hmp_savevm from savevm.c to hmp.c
> monitor: Move hmp_delvm from savevm.c to hmp.c
> monitor: Move hmp_info_snapshots from savevm.c to hmp.c
> migration: Pass Error ** argument to {save,load}_vmstate
>
> hmp.c | 179 +++++++++++++++++++++++++++++++++++++++
> hmp.h | 4 +
> include/sysemu/sysemu.h | 7 +-
> migration/savevm.c | 216 ++++++-----------------------------------------
> monitor.c | 13 ---
> replay/replay-snapshot.c | 6 +-
> vl.c | 4 +-
> 7 files changed, 217 insertions(+), 212 deletions(-)
>
> --
> 2.9.3
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 22+ messages in thread