From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37969) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVKWm-0004ko-Nn for qemu-devel@nongnu.org; Tue, 10 Mar 2015 09:48:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YVKWi-0001Fp-NN for qemu-devel@nongnu.org; Tue, 10 Mar 2015 09:48:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56030) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVKWi-0001Fj-Go for qemu-devel@nongnu.org; Tue, 10 Mar 2015 09:48:52 -0400 Date: Tue, 10 Mar 2015 14:48:46 +0100 From: Kevin Wolf Message-ID: <20150310134846.GE3770@noname.str.redhat.com> References: <20150225094112.GB18680@stefanha-thinkpad.redhat.com> <20150305174013.GA19499@stefanha-thinkpad.redhat.com> <20150306155017.GA2431@stefanha-thinkpad.redhat.com> <20150310132858.GB19057@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="qMm9M+Fa2AknHoGS" Content-Disposition: inline In-Reply-To: <20150310132858.GB19057@stefanha-thinkpad.redhat.com> Subject: Re: [Qemu-devel] [PATCH] savevm: create snapshot failed when id_str already exits List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: amit.shah@redhat.com, wang.yi59@zte.com.cn, Yi Wang , qemu-devel@nongnu.org, quintela@redhat.com --qMm9M+Fa2AknHoGS Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 10.03.2015 um 14:28 hat Stefan Hajnoczi geschrieben: > On Mon, Mar 09, 2015 at 09:32:47PM +0800, Yi Wang wrote: > > This will trigger the problem: vda has snapshot s1 with id "1", vdb > > doesn't have s1 but has snapshot s2 with id "1"=E3=80=82When we want to= run > > command "virsh create s1", del_existing_snapshots() only deletes s1 in > > vda, and bdrv_snapshot_create() tries to create vdb's snapshot s1 with > > id "1", but id "1" alreay exists in vdb with name "s2"! > >=20 > > This shows the condition: > > # qemu-img snapshot -l win7.img.1 > > Snapshot list: > > ID TAG VM SIZE DATE VM CLOCK > > 1 s1 534M 2015-03-09 10:28:54 00:03:54.504 > >=20 > > # qemu-img snapshot -l win7.append > > Snapshot list: > > ID TAG VM SIZE DATE VM CLOCK > > 1 s2 0 2015-03-05 10:29:21 00:00:00.000 > >=20 > > # virsh snapshot-create-as win7 s1 > > error: operation failed: Failed to take snapshot: Error while creating > > snapshot on 'drive-virtio-disk1' >=20 > Then we've arrived at changing the behavior of 'savevm' so it always > generates IDs. >=20 > Kevin: What do you think about changing savevm semantics to always > generate IDs? Sounds reasonable. We're free to set whatever ID we want. However, I wouldn't set id_str =3D "" like in the patch below, but remove the check qcow2_snapshot_create() and call find_new_snapshot_id() unconditionally. Kevin > diff --git a/savevm.c b/savevm.c > index ce2b6a2..bee2143 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -1141,7 +1141,6 @@ void hmp_savevm(Monitor *mon, const QDict *qdict) > ret =3D bdrv_snapshot_find(bs, old_sn, name); > if (ret >=3D 0) { > pstrcpy(sn->name, sizeof(sn->name), old_sn->name); > - pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str); > } else { > pstrcpy(sn->name, sizeof(sn->name), name); > } > @@ -1178,6 +1177,12 @@ void hmp_savevm(Monitor *mon, const QDict *qdict) > if (bdrv_can_snapshot(bs1)) { > /* Write VM state size only to the image that contains the s= tate */ > sn->vm_state_size =3D (bs =3D=3D bs1 ? vm_state_size : 0); > + > + /* Force ID generation for each image since there could be n= aming > + * collisions. > + */ > + sn->id_str[0] =3D '\0'; > + > ret =3D bdrv_snapshot_create(bs1, sn); > if (ret < 0) { > monitor_printf(mon, "Error while creating snapshot on '%= s'\n", --qMm9M+Fa2AknHoGS Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJU/vY+AAoJEH8JsnLIjy/WL2oQAMSMFFi+u/Dn0AbwOMxD4LJP kP6HXqK0K0qAzca4/9PcDMcHU+sCc/Llqfl09nVoFXz8gPoMLS2pwInfGUK7Ezol J7s4V9Ox5BjJOMFF3ZYfoIwWh+xoJJTGCqOBX2HylIS3J1ATPZOADH16TSS80xv5 r8FXEQ8ZeCpncDDR2ZryZNF9DVfgd5KSlaygtlzYOb4xUkxtLHeVr03zYPyTqZxz K8DuVHc9s2Qo3cvm3dzneKADu4opvlJ3LqaG0GGV2FHeRfytpV5DtCYfrP7O5x1k LeB3hEuuGzwdVhfK1KbgIlv3jKEUp8hzNTLUKQ2/LD8hn+j+dc6pe/yHn2qPmws0 xB5XJzAbPn5BEX7kBodAi7PEJrtWaXrvIFk3dJCH81zH2Z/S7mWpJJb0M7IJJhxp EXkvQ9mVwwHg/ZvC7PakLHbF3N6Gjq0NIKXWphzxDciqXVrGOzzbOAdkNbbNHgDG eOd0QuTx2iCNDr648RPsVfzOoLFOYO6eglRgbok8A0wMOtZqpRkFLE7gqR08YtzN LLedX++n6Iuo6seU9y5GAhICzZWs98tvRAJlQhI+6Bmfei+cLwPDaGsWC8t4Emz+ wJJlgdCa65ErID0D+QKA0ivFPTtGNJSIEHB9daQJ6dMvHLLn+922ZmKAmY0NHBhX oWiWXFIueX34y5iSL/8t =KJSs -----END PGP SIGNATURE----- --qMm9M+Fa2AknHoGS--