| Message ID | 582c2debbce809a86fe62d6a998d42bee8ef274c.1345016001.git.phrdina@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/15/2012 01:41 AM, Pavel Hrdina wrote: > HMP command "savevm" now takes extra optional force parameter to specifi whether s/specifi/specify/ > replace existing snapshot or not. > > QMP command "vm-snapshot-save" has also extra optional force parameter and > name parameter isn't optional anymore. When HMP omits the name, what name are you using in it's place? Either a nameless snapshot is okay (and QMP must expose it), or it is an error (and HMP must now be passing a reasonable name). /me reads patch Oh, there was always a name; the default name was a timestamp, and you moved the timestamp creation into HMP. > +++ b/hmp-commands.hx > @@ -264,19 +264,19 @@ ETEXI > > { > .name = "savevm", > - .args_type = "name:s?", > - .params = "[tag|id]", > - .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", > + .args_type = "name:s?,force:b?", > + .params = "[tag|id] [on|off]", Rather than sticking 'force' as an optional parameter at the end, instead you want to add '-f' as a flag at the beginning. .args_type = "force:-b,name:s?", .params = "[-f] name" By doing this, you no longer need 17/18. > + .help = "save a VM snapshot. To replace existing snapshot use force parameter.", > .mhandler.cmd = hmp_vm_snapshot_save, > }, > > STEXI > @item savevm [@var{tag}|@var{id}] > @findex savevm > -Create a snapshot of the whole virtual machine. If @var{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. More info at > -@ref{vm_snapshots}. > +Create a snapshot of the whole virtual machine. Parameter "name" is optional. > +If @var{tag} is provided, it is used as human readable identifier. If there is > +already a snapshot with the same tag, force argument need to be "yes" to s/force argument need/the force argument needs/ > +++ b/qapi-schema.json > @@ -2394,21 +2394,23 @@ > ## > # @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. > +# Create a snapshot of the whole virtual machine. Provided 'tag' is used as > +# human readable identifier. If there is already a snapshot with the same tag, > +# force argument need to be "yes" to replace it. s/need to be "yes"/needs to be 'true'/ > # > # 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 > +# @name: tag of new or existing snapshot Given your new semantics, @name must be tag when @force is false; but the old HMP semantics allowed 'savevm 1' to rewrite snapshot id 1 regardless of the name, all while keeping the old tag. Does the new HMP code do an id lookup, and pass in the correct @name to the QMP code? Should the QMP code allow the same semantics of being able to pass @force=true and @name=id instead of the normal creation of @name=tag? > +# > +# @force: specify whether existing snapshot is replaced or not Based on the JSON below, this needs an #optional marking, as well as mentioning that the default is false. > # > # Returns: Nothing on success > # If an error occurs, GenericError with error message Knowing that a creation failed due to a snapshot already existing by the same name or id might be worth a distinct error class (do we already have an error class that we could reuse?)
On 08/16/2012 07:00 AM, Eric Blake wrote: > On 08/15/2012 01:41 AM, Pavel Hrdina wrote: >> HMP command "savevm" now takes extra optional force parameter to specifi whether > s/specifi/specify/ > >> replace existing snapshot or not. >> >> QMP command "vm-snapshot-save" has also extra optional force parameter and >> name parameter isn't optional anymore. > When HMP omits the name, what name are you using in it's place? Either > a nameless snapshot is okay (and QMP must expose it), or it is an error > (and HMP must now be passing a reasonable name). > > /me reads patch > > Oh, there was always a name; the default name was a timestamp, and you > moved the timestamp creation into HMP. > > >> +++ b/hmp-commands.hx >> @@ -264,19 +264,19 @@ ETEXI >> >> { >> .name = "savevm", >> - .args_type = "name:s?", >> - .params = "[tag|id]", >> - .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", >> + .args_type = "name:s?,force:b?", >> + .params = "[tag|id] [on|off]", > Rather than sticking 'force' as an optional parameter at the end, > instead you want to add '-f' as a flag at the beginning. > > .args_type = "force:-b,name:s?", > .params = "[-f] name" > > By doing this, you no longer need 17/18. Thanks, I'll rewrite it for v2. >> + .help = "save a VM snapshot. To replace existing snapshot use force parameter.", >> .mhandler.cmd = hmp_vm_snapshot_save, >> }, >> >> STEXI >> @item savevm [@var{tag}|@var{id}] >> @findex savevm >> -Create a snapshot of the whole virtual machine. If @var{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. More info at >> -@ref{vm_snapshots}. >> +Create a snapshot of the whole virtual machine. Parameter "name" is optional. >> +If @var{tag} is provided, it is used as human readable identifier. If there is >> +already a snapshot with the same tag, force argument need to be "yes" to > s/force argument need/the force argument needs/ > >> +++ b/qapi-schema.json >> @@ -2394,21 +2394,23 @@ >> ## >> # @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. >> +# Create a snapshot of the whole virtual machine. Provided 'tag' is used as >> +# human readable identifier. If there is already a snapshot with the same tag, >> +# force argument need to be "yes" to replace it. > s/need to be "yes"/needs to be 'true'/ > >> # >> # 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 >> +# @name: tag of new or existing snapshot > Given your new semantics, @name must be tag when @force is false; but > the old HMP semantics allowed 'savevm 1' to rewrite snapshot id 1 > regardless of the name, all while keeping the old tag. Does the new HMP > code do an id lookup, and pass in the correct @name to the QMP code? > Should the QMP code allow the same semantics of being able to pass > @force=true and @name=id instead of the normal creation of @name=tag? You can override existing snapshot specified by id. I'll fix the documentation. >> +# >> +# @force: specify whether existing snapshot is replaced or not > Based on the JSON below, this needs an #optional marking, as well as > mentioning that the default is false. > >> # >> # Returns: Nothing on success >> # If an error occurs, GenericError with error message > Knowing that a creation failed due to a snapshot already existing by the > same name or id might be worth a distinct error class (do we already > have an error class that we could reuse?) > Regarding the Luiz's new error handling, for new errors we should use one error class.
diff --git a/hmp-commands.hx b/hmp-commands.hx index d6cd94c..58fd1f2 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -264,19 +264,19 @@ ETEXI { .name = "savevm", - .args_type = "name:s?", - .params = "[tag|id]", - .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", + .args_type = "name:s?,force:b?", + .params = "[tag|id] [on|off]", + .help = "save a VM snapshot. To replace existing snapshot use force parameter.", .mhandler.cmd = hmp_vm_snapshot_save, }, STEXI @item savevm [@var{tag}|@var{id}] @findex savevm -Create a snapshot of the whole virtual machine. If @var{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. More info at -@ref{vm_snapshots}. +Create a snapshot of the whole virtual machine. Parameter "name" is optional. +If @var{tag} is provided, it is used as human readable identifier. If there is +already a snapshot with the same tag, force argument need to be "yes" to +replace it. More info at @ref{vm_snapshots}. ETEXI { diff --git a/hmp.c b/hmp.c index cf0e036..14df8f1 100644 --- a/hmp.c +++ b/hmp.c @@ -1139,9 +1139,32 @@ void hmp_closefd(Monitor *mon, const QDict *qdict) void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict) { const char *name = qdict_get_try_str(qdict, "name"); + char new_name[256]; + bool force = qdict_get_try_bool(qdict, "force", 0); Error *err = NULL; +#ifdef _WIN32 + struct _timeb tb; + struct tm *ptm; +#else + struct timeval tv; + struct tm tm; +#endif + + if (!name) { +#ifdef _WIN32 + time_t t = tb.time; + ptm = localtime(&t); + strftime(new_name, sizeof(new_name), "vm-%Y%m%d%H%M%S", ptm); +#else + /* cast below needed for OpenBSD where tv_sec is still 'long' */ + localtime_r((const time_t *)&tv.tv_sec, &tm); + strftime(new_name, sizeof(new_name), "vm-%Y%m%d%H%M%S", &tm); +#endif + } else { + pstrcpy(new_name, sizeof(new_name), name); + } - qmp_vm_snapshot_save(!!name, name, &err); + qmp_vm_snapshot_save(new_name, !!force, force, &err); hmp_handle_error(mon, &err); } diff --git a/qapi-schema.json b/qapi-schema.json index 763b51e..e68b259 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2394,21 +2394,23 @@ ## # @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. +# Create a snapshot of the whole virtual machine. Provided 'tag' is used as +# human readable identifier. If there is already a snapshot with the same tag, +# force argument need to be "yes" to replace it. # # 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 +# @name: tag of new or existing snapshot +# +# @force: specify whether existing snapshot is replaced or not # # Returns: Nothing on success # If an error occurs, GenericError with error message # # Since: 1.2 ## -{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} } +{ 'command': 'vm-snapshot-save', 'data': {'name': 'str', '*force': 'bool'} } ## # @vm-snapshot-load: diff --git a/qmp-commands.hx b/qmp-commands.hx index 3609de8..c86ceb5 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1112,9 +1112,9 @@ Example: EQMP { .name = "vm-snapshot-save", - .args_type = "name:s?", - .params = "name", - .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", + .args_type = "name:s,force:b?", + .params = "name [force]", + .help = "save a VM snapshot. To replace existing snapshot use force parameter.", .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_save }, @@ -1122,16 +1122,18 @@ 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. +Create a snapshot of the whole virtual machine. Provided 'tag' is used as +human readable identifier. If there is already a snapshot with the same tag, +force argument need to be "yes" to replace it. The VM is automatically stopped and resumed and saving a snapshot can take a long time. Arguments: -- "name": tag or id of new or existing snapshot +- "name": tag of new or existing snapshot + +- "force": specify whether existing snapshot is replaced or not Example: diff --git a/savevm.c b/savevm.c index ca7448d..4ec7663 100644 --- a/savevm.c +++ b/savevm.c @@ -2100,7 +2100,7 @@ static int del_existing_snapshots(const char *name, return 0; } -void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp) +void qmp_vm_snapshot_save(const char *name, bool has_force, bool force, Error **errp) { BlockDriverState *bs, *bs1; QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1; @@ -2110,10 +2110,8 @@ void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp) uint64_t vm_state_size; #ifdef _WIN32 struct _timeb tb; - struct tm *ptm; #else struct timeval tv; - struct tm tm; #endif /* Verify if there is a device that doesn't support snapshots and is writable */ @@ -2153,29 +2151,24 @@ void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp) #endif sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock); - if (has_name) { - ret = bdrv_snapshot_find(bs, old_sn, name, NULL); - if (ret >= 0) { + ret = bdrv_snapshot_find(bs, old_sn, name, NULL); + if (ret >= 0) { + if (has_force && force) { pstrcpy(sn->name, sizeof(sn->name), old_sn->name); pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str); + + del_existing_snapshots(name, errp); + if (error_is_set(errp)) { + goto the_end; + } } else { - pstrcpy(sn->name, sizeof(sn->name), name); + error_set(errp, ERROR_CLASS_GENERIC_ERROR, + "Snapshot '%s' exist. For override specify force=yes", + name); + goto the_end; } } else { -#ifdef _WIN32 - time_t t = tb.time; - ptm = localtime(&t); - strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", ptm); -#else - /* cast below needed for OpenBSD where tv_sec is still 'long' */ - localtime_r((const time_t *)&tv.tv_sec, &tm); - strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm); -#endif - } - - /* Delete old snapshots of the same name */ - if (has_name && del_existing_snapshots(name, errp) < 0) { - goto the_end; + pstrcpy(sn->name, sizeof(sn->name), name); } /* save the VM state */
HMP command "savevm" now takes extra optional force parameter to specifi whether replace existing snapshot or not. QMP command "vm-snapshot-save" has also extra optional force parameter and name parameter isn't optional anymore. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- hmp-commands.hx | 14 +++++++------- hmp.c | 25 ++++++++++++++++++++++++- qapi-schema.json | 12 +++++++----- qmp-commands.hx | 16 +++++++++------- savevm.c | 35 ++++++++++++++--------------------- 5 files changed, 61 insertions(+), 41 deletions(-)