Comments

On 08/15/2012 01:41 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>> ---
I'm focusing my review more on the public interface (since that's what
affects libvirt), and therefore glanced through 1 through 12 but did not
pay close attention to them.
> hmp-commands.hx | 2 +-> hmp.c | 10 ++++++++++> hmp.h | 1 +> qapi-schema.json | 19 +++++++++++++++++++> qmp-commands.hx | 29 +++++++++++++++++++++++++++++> savevm.c | 25 +++++++++----------------> sysemu.h | 1 -> 7 files changed, 69 insertions(+), 18 deletions(-)> > +> +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)> +{> + const char *name = qdict_get_try_str(qdict, "name");
In the cover letter, you said "and for QMP you have to always provide
name parameter" - but this says 'name' is optional and can still be
empty (id only).
> +++ b/qapi-schema.json> @@ -2356,3 +2356,22 @@> # Since: 1.2.0> ##> { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'] }> +> +##> +# @vm-snapshot-save:> +#> +# Create a snapshot of the whole virtual machine. If 'tag' is provided,
'tag' is a misnomer, since you list @name as the parameter name.
> +# it is used as human readable identifier. If there is already a snapshot> +# with the same tag or ID, it is replaced.> +#> +# The VM is automatically stopped and resumed and saving a snapshot can take> +# a long time.> +#> +# @name: tag or id of new or existing snapshot
Here, you document @name as required,
> +#> +# Returns: Nothing on success> +# If an error occurs, GenericError with error message> +#> +# Since: 1.2
We missed 1.2 hard freeze. This better be 1.3.
> +##> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
but here, you listed it as '*name' meaning optional. I'm okay with
leaving name optional, provided that it means that you end up allocating
the next unique id. But if that's the case, then returning nothing is
wrong; this command needs to return { 'id':'int', '*name':'str' }, so
that the user knows what snapshot just got allocated.
> +SQMP> +vm-snapshot-save> +------> +> +Create a snapshot of the whole virtual machine. If 'tag' is provided,> +it is used as human readable identifier. If there is already a snapshot> +with the same tag or ID, it is replaced.> +> +The VM is automatically stopped and resumed and saving a snapshot can take> +a long time.
I don't like that this command can take a long time. Just today on
#virt IRC, someone was complaining that 'savevm' HMP cannot be canceled.
If we're going to create a new interface, it would be nicer to create a
command that starts the save process and returns immediately, as well as
commands to track progress, allow an early abort, and send an event on
completion. The HMP 'savevm' interface can issue multiple QMP commands
under the hood to continue with it's blocking behavior, but as long as
we are fixing things, I think the QMP interface should be more powerful.
> +> +Arguments:> +> +- "name": tag or id of new or existing snapshot> +> +Example:> +> +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } }> +<- { "return": {} }
Again, you need to return the id of the just-created snapshot.
> @@ -2176,21 +2174,20 @@ void do_savevm(Monitor *mon, const QDict *qdict)> }> > /* Delete old snapshots of the same name */> - if (name && del_existing_snapshots(name, NULL) < 0) {> + if (has_name && del_existing_snapshots(name, errp) < 0) {
Here's hoping later in the series updates this to make saner decisions.
(Maybe I should peruse the entire series before commenting on
individual patches?)

On 08/15/2012 09:49 PM, Eric Blake wrote:
> On 08/15/2012 01:41 AM, Pavel Hrdina wrote:>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>>> ---> I'm focusing my review more on the public interface (since that's what> affects libvirt), and therefore glanced through 1 through 12 but did not> pay close attention to them.>>> hmp-commands.hx | 2 +->> hmp.c | 10 ++++++++++>> hmp.h | 1 +>> qapi-schema.json | 19 +++++++++++++++++++>> qmp-commands.hx | 29 +++++++++++++++++++++++++++++>> savevm.c | 25 +++++++++---------------->> sysemu.h | 1 ->> 7 files changed, 69 insertions(+), 18 deletions(-)>>>> +>> +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)>> +{>> + const char *name = qdict_get_try_str(qdict, "name");> In the cover letter, you said "and for QMP you have to always provide> name parameter" - but this says 'name' is optional and can still be> empty (id only).
I said in cover letter that the last two patches introduce this
functionality.
>> +++ b/qapi-schema.json>> @@ -2356,3 +2356,22 @@>> # Since: 1.2.0>> ##>> { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'] }>> +>> +##>> +# @vm-snapshot-save:>> +#>> +# Create a snapshot of the whole virtual machine. If 'tag' is provided,> 'tag' is a misnomer, since you list @name as the parameter name.>>> +# it is used as human readable identifier. If there is already a snapshot>> +# with the same tag or ID, it is replaced.>> +#>> +# The VM is automatically stopped and resumed and saving a snapshot can take>> +# a long time.>> +#>> +# @name: tag or id of new or existing snapshot> Here, you document @name as required,>>> +#>> +# Returns: Nothing on success>> +# If an error occurs, GenericError with error message>> +#>> +# Since: 1.2> We missed 1.2 hard freeze. This better be 1.3.>>> +##>> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }> but here, you listed it as '*name' meaning optional. I'm okay with> leaving name optional, provided that it means that you end up allocating> the next unique id. But if that's the case, then returning nothing is> wrong; this command needs to return { 'id':'int', '*name':'str' }, so> that the user knows what snapshot just got allocated.
If you apply whole patch-series, then you have to for QMP always provide
@name.
As I said above, this behaviour is introduced by last two patches.
>> +SQMP>> +vm-snapshot-save>> +------>> +>> +Create a snapshot of the whole virtual machine. If 'tag' is provided,>> +it is used as human readable identifier. If there is already a snapshot>> +with the same tag or ID, it is replaced.>> +>> +The VM is automatically stopped and resumed and saving a snapshot can take>> +a long time.> I don't like that this command can take a long time. Just today on> #virt IRC, someone was complaining that 'savevm' HMP cannot be canceled.> If we're going to create a new interface, it would be nicer to create a> command that starts the save process and returns immediately, as well as> commands to track progress, allow an early abort, and send an event on> completion. The HMP 'savevm' interface can issue multiple QMP commands> under the hood to continue with it's blocking behavior, but as long as> we are fixing things, I think the QMP interface should be more powerful.
Well, I could try to do it...with this I'll probably introduce new
command vm-snapshot-save-cancel or vm-snaphsot-cancel as we have migrate
and migrate-cancel.
>> +>> +Arguments:>> +>> +- "name": tag or id of new or existing snapshot>> +>> +Example:>> +>> +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } }>> +<- { "return": {} }> Again, you need to return the id of the just-created snapshot.>>> @@ -2176,21 +2174,20 @@ void do_savevm(Monitor *mon, const QDict *qdict)>> }>> >> /* Delete old snapshots of the same name */>> - if (name && del_existing_snapshots(name, NULL) < 0) {>> + if (has_name && del_existing_snapshots(name, errp) < 0) {> Here's hoping later in the series updates this to make saner decisions.> (Maybe I should peruse the entire series before commenting on> individual patches?)>