| Message ID | ff0fc38e29e321f5799c1a57a179df7d4d4fdb1d.1357741229.git.phrdina@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/09/2013 08:18 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. The qcow2 format lets 'name' be optional; why are we insisting at the QMP layer that we cannot use this feature? It may be a change worth making, but you'll need more arguments in the commit message; and you'll still need to make sure that qemu can handle an arbitrary qcow2 file that has an existing snapshot created by older qemu when name was still optional. > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > hmp-commands.hx | 15 ++++++++------- > hmp.c | 25 ++++++++++++++++++++++++- > qapi-schema.json | 9 ++++++--- > qmp-commands.hx | 15 +++++++++------ > savevm.c | 36 ++++++++++++++---------------------- > 5 files changed, 61 insertions(+), 39 deletions(-) > > +@item savevm [@var{-f}] @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 @var{tag} or @var{id}, it is replaced. More info at > +Create a snapshot of the whole virtual machine. Paramtere "name" is optional. s/Paramtere/Parameter/ > +++ b/hmp.c > @@ -1340,8 +1340,31 @@ void hmp_nbd_server_stop(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); It feels like lots of pending patches are trying to touch this same area of code; make sure you coordinate the efforts (for example, http://patchwork.ozlabs.org/patch/210250/ should go in first). > +++ b/qapi-schema.json > @@ -3023,15 +3023,18 @@ > # > # Create a snapshot of the whole virtual machine. If tag is provided as @name, > # it is used as human readable identifier. If there is already a snapshot > -# with the same tag or ID, it is replaced. > +# with the same tag or id, the force argument needs to be true to replace it. > # > # The VM is automatically stopped and resumed and saving a snapshot can take > # a long time. > # > -# @name: #optional tag of new snapshot or tag|id of existing snapshot > +# @name: tag of new snapshot or tag|id of existing snapshot It feels like you are doing too much in this commit. Either name should never have been optional earlier in the series, or the change to make name mandatory should be its own commit (since nothing in the one-line summary mentions making name mandatory). > + 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); > + > + /* Delete old snapshots of the same name */ > + if (del_existing_snapshots(name, &local_err) < 0) { > + error_propagate(errp, local_err); > + goto the_end; > + } > } else { > - pstrcpy(sn->name, sizeof(sn->name), name); > + error_setg(errp, "Snapshot '%s' exist. For override add " s/exist/exists/
On Wed, 2013-01-09 at 15:47 -0700, Eric Blake wrote: > On 01/09/2013 08:18 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. > > The qcow2 format lets 'name' be optional; why are we insisting at the > QMP layer that we cannot use this feature? It may be a change worth > making, but you'll need more arguments in the commit message; and you'll > still need to make sure that qemu can handle an arbitrary qcow2 file > that has an existing snapshot created by older qemu when name was still > optional. > Based on your comment I think that we should create this behavior: We will have for HMP and QMP two parameters id and name and you can choose only one of them next to the force option. If name is specified and there is a snapshot with specified name, the snapshot will be overwritten. If name is specified and there isn't a snapshot with specified name, a new snapshot will be written. If id is specified and there is a snapshot with specified id, the snapshot will be overwritten. If id is specified and there isn't a snapshot with specified id, the command ends with error message that there isn't snapshot to overwrite with specified id. If id or name is not specified the new snapshot with generated name and id will be created. In all successful cases the information about snapshot will be returned. > > > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > > --- > > hmp-commands.hx | 15 ++++++++------- > > hmp.c | 25 ++++++++++++++++++++++++- > > qapi-schema.json | 9 ++++++--- > > qmp-commands.hx | 15 +++++++++------ > > savevm.c | 36 ++++++++++++++---------------------- > > 5 files changed, 61 insertions(+), 39 deletions(-) > > > > > +@item savevm [@var{-f}] @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 @var{tag} or @var{id}, it is replaced. More info at > > +Create a snapshot of the whole virtual machine. Paramtere "name" is optional. > > s/Paramtere/Parameter/ > > > +++ b/hmp.c > > @@ -1340,8 +1340,31 @@ void hmp_nbd_server_stop(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); > > It feels like lots of pending patches are trying to touch this same area > of code; make sure you coordinate the efforts (for example, > http://patchwork.ozlabs.org/patch/210250/ should go in first). > > > +++ b/qapi-schema.json > > @@ -3023,15 +3023,18 @@ > > # > > # Create a snapshot of the whole virtual machine. If tag is provided as @name, > > # it is used as human readable identifier. If there is already a snapshot > > -# with the same tag or ID, it is replaced. > > +# with the same tag or id, the force argument needs to be true to replace it. > > # > > # The VM is automatically stopped and resumed and saving a snapshot can take > > # a long time. > > # > > -# @name: #optional tag of new snapshot or tag|id of existing snapshot > > +# @name: tag of new snapshot or tag|id of existing snapshot > > It feels like you are doing too much in this commit. Either name should > never have been optional earlier in the series, or the change to make > name mandatory should be its own commit (since nothing in the one-line > summary mentions making name mandatory). > > > + 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); > > + > > + /* Delete old snapshots of the same name */ > > + if (del_existing_snapshots(name, &local_err) < 0) { > > + error_propagate(errp, local_err); > > + goto the_end; > > + } > > } else { > > - pstrcpy(sn->name, sizeof(sn->name), name); > > + error_setg(errp, "Snapshot '%s' exist. For override add " > > s/exist/exists/ >
diff --git a/hmp-commands.hx b/hmp-commands.hx index 3b781f7..d857682 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -307,18 +307,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 = "force:-f,name:s", + .params = "[-f] [tag|id]", + .help = "save a VM snapshot. To replace existing snapshot use force flag.", .mhandler.cmd = hmp_vm_snapshot_save, }, STEXI -@item savevm [@var{tag}|@var{id}] +@item savevm [@var{-f}] @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 @var{tag} or @var{id}, it is replaced. More info at +Create a snapshot of the whole virtual machine. Paramtere "name" is optional. +If @var{tag} is provided, it is used as human readable identifier. If there is +already a snapshot with the same @var{tag} or @var{id}, @var{-f} flag needs to +be specified. More info at @ref{vm_snapshots}. @ref{vm_snapshots}. ETEXI diff --git a/hmp.c b/hmp.c index 2d8600a..02493ca 100644 --- a/hmp.c +++ b/hmp.c @@ -1340,8 +1340,31 @@ void hmp_nbd_server_stop(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 a6f4cb0..7f1639c 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3023,15 +3023,18 @@ # # Create a snapshot of the whole virtual machine. If tag is provided as @name, # it is used as human readable identifier. If there is already a snapshot -# with the same tag or ID, it is replaced. +# with the same tag or id, the force argument needs to be true to replace it. # # The VM is automatically stopped and resumed and saving a snapshot can take # a long time. # -# @name: #optional tag of new snapshot or tag|id of existing snapshot +# @name: tag of new snapshot or tag|id of existing snapshot +# +# @force: #optional specify whether existing snapshot is replaced or not, +# default is false # # Returns: Nothing on success # # Since: 1.4.0 ## -{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} } +{ 'command': 'vm-snapshot-save', 'data': {'name': 'str', '*force': 'bool'} } diff --git a/qmp-commands.hx b/qmp-commands.hx index 9a0b566..1bd16f1 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1349,9 +1349,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 }, @@ -1360,15 +1360,18 @@ vm-snapshot-save ------ Create a snapshot of the whole virtual machine. If tag is provided as name, -it is used as human readable identifier. If there is already a snapshot -with the same tag or id, it is replaced. +it is used as human readable identifier. If there is already a snapshot with +the same tag, the force argument needs to be true to replace it. The VM is automatically stopped and resumed and saving a snapshot can take a long time. Arguments: -- "name": #optional tag of new snapshot or tag|id of existing snapshot (json-string, optional) +- "name": tag of new snapshot or tag|id of existing snapshot (json-string) + +- "force": specify whether existing snapshot is replaced or not, + default is false (json-bool, optional) Example: diff --git a/savevm.c b/savevm.c index a7254b2..090ab1f 100644 --- a/savevm.c +++ b/savevm.c @@ -2102,7 +2102,7 @@ static int del_existing_snapshots(const char *name, Error **errp) 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; @@ -2112,10 +2112,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 Error *local_err = NULL; @@ -2157,30 +2155,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); - if (ret >= 0) { + ret = bdrv_snapshot_find(bs, old_sn, name); + 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); + + /* Delete old snapshots of the same name */ + if (del_existing_snapshots(name, &local_err) < 0) { + error_propagate(errp, local_err); + goto the_end; + } } else { - pstrcpy(sn->name, sizeof(sn->name), name); + error_setg(errp, "Snapshot '%s' exist. For override add " + "'force' parameter.", 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, &local_err) < 0) { - error_propagate(errp, local_err); - 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 | 15 ++++++++------- hmp.c | 25 ++++++++++++++++++++++++- qapi-schema.json | 9 ++++++--- qmp-commands.hx | 15 +++++++++------ savevm.c | 36 ++++++++++++++---------------------- 5 files changed, 61 insertions(+), 39 deletions(-)