From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45795) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tt4QN-0003Yp-3D for qemu-devel@nongnu.org; Wed, 09 Jan 2013 17:47:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tt4QL-0007KK-IM for qemu-devel@nongnu.org; Wed, 09 Jan 2013 17:47:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:62214) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tt4QL-0007KG-AR for qemu-devel@nongnu.org; Wed, 09 Jan 2013 17:47:05 -0500 Message-ID: <50EDF364.80905@redhat.com> Date: Wed, 09 Jan 2013 15:47:00 -0700 From: Eric Blake MIME-Version: 1.0 References: In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enigE24660A9C38A7E0CE9263649" Subject: Re: [Qemu-devel] [PATCH 13/13] vm-snapshot-save: add force parameter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Hrdina Cc: qemu-devel@nongnu.org, phrdina@redhat.c0m, lcapitulino@redhat.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigE24660A9C38A7E0CE9263649 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 01/09/2013 08:18 AM, Pavel Hrdina wrote: > HMP command "savevm" now takes extra optional force parameter to specif= i whether s/specifi/specify/ > replace existing snapshot or not. >=20 > 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. >=20 > Signed-off-by: Pavel Hrdina > --- > 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(-) >=20 > +@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 i= nfo at > +Create a snapshot of the whole virtual machine. Paramtere "name" is op= tional. s/Paramtere/Parameter/ > +++ b/hmp.c > @@ -1340,8 +1340,31 @@ void hmp_nbd_server_stop(Monitor *mon, const QDi= ct *qdict) > void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict) > { > const char *name =3D qdict_get_try_str(qdict, "name"); > + char new_name[256]; > + bool force =3D qdict_get_try_bool(qdict, "force", 0); > Error *err =3D NULL; > +#ifdef _WIN32 > + struct _timeb tb; > + struct tm *ptm; > +#else > + struct timeval tv; > + struct tm tm; > +#endif > + > + if (!name) { > +#ifdef _WIN32 > + time_t t =3D tb.time; > + ptm =3D 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 a= s @name, > # it is used as human readable identifier. If there is already a snaps= hot > -# with the same tag or ID, it is replaced. > +# with the same tag or id, the force argument needs to be true to repl= ace it. > # > # The VM is automatically stopped and resumed and saving a snapshot ca= n 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 >=3D 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/ --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enigE24660A9C38A7E0CE9263649 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with undefined - http://www.enigmail.net/ iQEcBAEBCAAGBQJQ7fNlAAoJEKeha0olJ0Nq0KIH/j9VzytU6JmQz02Tl8ni6aC+ 7LglqNN+tWHnu547Ed8ggVY2kEW4+CeEi7WWrTk10rapXxJQ9KJAEZ8cWH6TFsYV 4e8xeNKPU0L4d49Eemq4C/ZmJbqQGADqyys36wATJ9UrYPN6tu0E5Q+CHQvWYkRH N5QSXTq9AqU4uvDFR0XuHwXXEnkapZRmDK5F7eLCupOaQ2/llmRpnFhZJKl+skj7 SCqKPRzEc9EWpMCN2NHtDhh1SoIPspuPrclJA+AN6ZfIj/rUI6m9LVzD/rYzuw18 oWdci4CxpBMrNi0me1z0IiauNassBKFqibXygAkY3BdeNVsHYRQAgUuwtkmrdDQ= =pItz -----END PGP SIGNATURE----- --------------enigE24660A9C38A7E0CE9263649--