From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58105) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USPxm-0006jE-4J for qemu-devel@nongnu.org; Wed, 17 Apr 2013 06:51:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1USPxk-0000vT-3D for qemu-devel@nongnu.org; Wed, 17 Apr 2013 06:51:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51785) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USPxj-0000vF-QU for qemu-devel@nongnu.org; Wed, 17 Apr 2013 06:51:39 -0400 Message-ID: <516E7EB1.8060806@redhat.com> Date: Wed, 17 Apr 2013 12:51:29 +0200 From: Pavel Hrdina MIME-Version: 1.0 References: <13b9d1e79947b89982ec51c421b9b1bd0a7b587d.1366127809.git.phrdina@redhat.com> <516E0E9A.9050607@linux.vnet.ibm.com> <516E54AF.8070206@redhat.com> <516E774D.8080809@linux.vnet.ibm.com> In-Reply-To: <516E774D.8080809@linux.vnet.ibm.com> Content-Type: text/plain; charset=GB2312 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 11/11] savevm: remove backward compatibility from bdrv_snapshot_find() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: armbru@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com On 17.4.2013 12:19, Wenchao Xia wrote: > =D3=DA 2013-4-17 15:52, Pavel Hrdina =D0=B4=B5=C0: >> Hi Wenchao, >> >> unfortunately no. According to new design of savevm, loadvm and delvm = I >> need also search for snapshots that have the specified name and id. >> > It seems the logic in your function, is same with mine... It is not the same. Your logic: if id is set: if there is snapshot with that id: end searching if name set (search also if id is set but nothing found): if there is snapshot with that name: end searching My logic: if name is set and id is set: if there is snapshot with than name and with that id: end searching else if name is set (means that only name is set): if there is snapshot with that name: end searching else if id is set (means that only id is set): if there is snapshot with that id: end searching >=20 >> I'm also touching bdrv_snapshot_list where I'm adding an Error paramet= er > I looked it before, but it needs all call back in ./block support it= , > so is it really necessary? I think it is better if this function internally set appropriate error message based on used disk image format (qcow2, sheepdog, rbd). Adding to CC Eric for his opinion. >=20 >> and changing the return value to be used only for getting a number of >> snapshots. So in case that there is some error, the return value will = be 0. >> >> Pavel >> >> On 17.4.2013 04:53, Wenchao Xia wrote: >>> Hi, Pavel >>> I have implemented it at >>> http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg02533.html >>> in patch 1,2. Could u check if it satisfy your requirement, if yes ma= ybe >>> you can directly use them. >>> >>>> Signed-off-by: Pavel Hrdina >>>> --- >>>> savevm.c | 33 +++++++++++---------------------- >>>> 1 file changed, 11 insertions(+), 22 deletions(-) >>>> >>>> diff --git a/savevm.c b/savevm.c >>>> index 66753da..bc829a5 100644 >>>> --- a/savevm.c >>>> +++ b/savevm.c >>>> @@ -2195,7 +2195,7 @@ out: >>>> } >>>> >>>> static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapsho= tInfo *sn_info, >>>> - const char *name, const char *id, boo= l old_match) >>>> + const char *name, const char *id) >>>> { >>>> QEMUSnapshotInfo *sn_tab, *sn; >>>> int nb_sns, i, found =3D 0; >>>> @@ -2218,20 +2218,10 @@ static int bdrv_snapshot_find(BlockDriverSta= te *bs, QEMUSnapshotInfo *sn_info, >>>> break; >>>> } >>>> } else if (name) { >>>> - /* for compatibility for old bdrv_snapshot_find call >>>> - * will be removed */ >>>> - if (old_match) { >>>> - if (!strcmp(sn->id_str, id) || !strcmp(sn->name, na= me)) { >>>> - *sn_info =3D *sn; >>>> - found =3D 1; >>>> - break; >>>> - } >>>> - } else { >>>> - if (!strcmp(sn->name, name)) { >>>> - *sn_info =3D *sn; >>>> - found =3D 1; >>>> - break; >>>> - } >>>> + if (!strcmp(sn->name, name)) { >>>> + *sn_info =3D *sn; >>>> + found =3D 1; >>>> + break; >>>> } >>>> } else if (id) { >>>> if (!strcmp(sn->id_str, id)) { >>>> @@ -2290,7 +2280,7 @@ SnapshotInfo *qmp_vm_snapshot_save(const char = *name, Error **errp) >>>> sn->date_nsec =3D tv.tv_usec * 1000; >>>> sn->vm_clock_nsec =3D qemu_get_clock_ns(vm_clock); >>>> >>>> - if (bdrv_snapshot_find(bs, old_sn, name, NULL, false)) { >>>> + if (bdrv_snapshot_find(bs, old_sn, name, NULL)) { >>>> error_setg(errp, "snapshot '%s' exists", name); >>>> goto the_end; >>>> } else { >>>> @@ -2388,7 +2378,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_na= me, const char *name, >>>> } >>>> >>>> /* Don't even try to load empty VM states */ >>>> - if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, false)) { >>>> + if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id)) { >>>> return NULL; >>>> } >>>> >>>> @@ -2413,7 +2403,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_na= me, const char *name, >>>> return NULL; >>>> } >>>> >>>> - if (!bdrv_snapshot_find(bs, &sn, name, id, false)) { >>>> + if (!bdrv_snapshot_find(bs, &sn, name, id)) { >>>> return NULL; >>>> } >>>> } >>>> @@ -2484,7 +2474,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const boo= l has_name, const char *name, >>>> return NULL; >>>> } >>>> >>>> - if (!bdrv_snapshot_find(bs, &sn, name, id, false)) { >>>> + if (!bdrv_snapshot_find(bs, &sn, name, id)) { >>>> /* no need to set an error if snapshot doesn't exist */ >>>> return NULL; >>>> } >>>> @@ -2501,7 +2491,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const boo= l has_name, const char *name, >>>> bs =3D NULL; >>>> while ((bs =3D bdrv_next(bs))) { >>>> if (bdrv_can_snapshot(bs) >>>> - && bdrv_snapshot_find(bs, &sn, name, id, false)) { >>>> + && bdrv_snapshot_find(bs, &sn, name, id)) { >>>> bdrv_snapshot_delete(bs, sn.name, errp); >>>> if (error_is_set(errp)) { >>>> return NULL; >>>> @@ -2549,8 +2539,7 @@ void do_info_snapshots(Monitor *mon, const QDi= ct *qdict) >>>> >>>> while ((bs1 =3D bdrv_next(bs1))) { >>>> if (bdrv_can_snapshot(bs1) && bs1 !=3D bs) { >>>> - if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn-= >id_str, >>>> - true)) { >>>> + if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn-= >id_str)) { >>>> available =3D 0; >>>> break; >>>> } >>>> >>> >>> >> >> >=20 >=20