Re: [Qemu-devel] [PATCH 1/2] Add save-snapshot, load-snapshot and delete-snapshot to QAPI

[Peter Xu]

On Sun, Jan 07, 2018 at 01:23:35PM +0100, Richard Palethorpe wrote:
> Add QAPI wrapper functions for the existing snapshot functionality. These
> functions behave the same way as the HMP savevm, loadvm and delvm
> commands. This will allow applications, such as OpenQA, to programmatically
> revert the VM to a previous state with no dependence on HMP or qemu-img.
> 
> I used the term snapshot instead of VM because I think it is less ambiguous
> and matches the internal function names.
> 
> Signed-off-by: Richard Palethorpe <address@hidden>
> ---
>  migration/savevm.c | 27 ++++++++++++++++++++++
>  qapi-schema.json   | 66 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 93 insertions(+)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index b7908f62be..d7bc0f0d41 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2242,6 +2242,11 @@ int save_snapshot(const char *name, Error **errp)
>      return ret;
>  }
>  
> +void qmp_save_snapshot(const char *name, Error **errp)
> +{
> +    save_snapshot(name, errp);
> +}
> +
>  void qmp_xen_save_devices_state(const char *filename, bool has_live, bool 
> live,
>                                  Error **errp)
>  {
> @@ -2404,6 +2409,28 @@ err_drain:
>      return ret;
>  }
>  
> +void qmp_load_snapshot(const char *name, Error **errp)
> +{
> +    int saved_vm_running = runstate_is_running();
> +
> +    vm_stop(RUN_STATE_RESTORE_VM);
> +
> +    if (!load_snapshot(name, errp) && saved_vm_running) {
> +        vm_start();
> +    }
> +}

Hi, Richard,

I think it's good to have, but from interface POV for sure I'll leave
it for maintainers.

For the code, I would prefer at least unify the code instead of
duplicating it.  I think calling qmp_*() functions in hmp_*() would be
good since that's what we do now mostly AFAICT.

Also, comparing to exposing snapshot operations, I am curious about
what would be the difference if we just migrate (which can use QMP) to
a file and then load that file using incoming migration.  Thanks,

-- 
Peter Xu