Re: [Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Eric Blake]

On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
> 'name' attribute is made mandatory in distinction with HMP command.
> 
> The patch also moves hmp_savevm implementation into hmp.c. This function
> is just a simple wrapper now and does not have knowledge about
> migration internals.
> 
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Juan Quintela <address@hidden>
> CC: Amit Shah <address@hidden>
> CC: Markus Armbruster <address@hidden>
> CC: Eric Blake <address@hidden>
> ---
>  hmp.c              | 12 ++++++++++++
>  migration/savevm.c | 13 +------------
>  qapi-schema.json   | 13 +++++++++++++
>  qmp-commands.hx    | 25 +++++++++++++++++++++++++
>  4 files changed, 51 insertions(+), 12 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 2140605..c9c7100 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -32,6 +32,7 @@
>  #include "ui/console.h"
>  #include "block/qapi.h"
>  #include "qemu-io.h"
> +#include "sysemu/sysemu.h"

What is this header needed for?

>  
>  #ifdef CONFIG_SPICE
>  #include <spice/enums.h>
> @@ -2378,3 +2379,14 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const 
> QDict *qdict)
>  
>      qapi_free_RockerOfDpaGroupList(list);
>  }
> +
> +void hmp_savevm(Monitor *mon, const QDict *qdict)
> +{
> +    Error *local_err = NULL;

I've been favoring 'err' rather than 'local_err', but that's not a
show-stopper since we still have both spellings mixed in the tree.

> +
> +    qmp_savevm(qdict_get_try_str(qdict, "name"), &local_err);

qdict_get_try_str() can return NULL;, but qmp_savevm() requires a
non-NULL string.  You'll need to invent a name in HMP if the user didn't
give you one.

> +++ b/migration/savevm.c
> @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
>      return ret;
>  }
>  
> -static void do_savevm(const char *name, Error **errp)
> +void qmp_savevm(const char *name, Error **errp)
>  {

This function still has some 'if (name)' conditionals; but according to
the qapi contract, name should never be NULL. You need to hoist the name
creation aspect to the caller (hmp_savevm), and in qmp_savevm you can
then assert(name).

>      BlockDriverState *bs, *bs1;
>      QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> @@ -1999,17 +1999,6 @@ static void do_savevm(const char *name, Error **errp)
>      }
>  }
>  
> -void hmp_savevm(Monitor *mon, const QDict *qdict)
> -{
> -    Error *local_err = NULL;
> -
> -    do_savevm(qdict_get_try_str(qdict, "name"), &local_err);

Hmm.  You are doing code motion of code you just added in 1/5; wouldn't
it be better if 1/5 stuck it in the right file to begin with?  And my
argument about needing the name logic in HMP (not QMP) means that maybe
the split in 1/5 isn't ideal, after all.

> +++ b/qapi-schema.json
> @@ -3971,3 +3971,16 @@
>  ##
>  { 'enum': 'ReplayMode',
>    'data': [ 'none', 'record', 'play' ] }
> +
> +##
> +# @savevm
> +#
> +# Save a VM snapshot. Without a name new snapshot is created",

Stale sentence, since a name is required.

> +#
> +# @name: identifier of a snapshot to be created
> +#
> +# Returns: Nothing on success
> +#
> +# Since 2.6
> +##
> +{ 'command': 'savevm', 'data': {'name': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 9d8b42f..f216c5e 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4739,3 +4739,28 @@ Example:
>                   {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>                    "pop-vlan": 1, "id": 251658240}
>     ]}
> +
> +EQMP
> +
> +SQMP
> +savevm
> +------------------

Line too long. Make it match the length of the line above.

> +
> +Save a VM snapshot. If no tag or id are provided, a new snapshot is created

Stale sentence, since you are requiring a name.

Why does QMP not allow specifying an id?  Should it?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature