From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53471) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVFtR-0000rW-Ka for qemu-devel@nongnu.org; Thu, 25 Apr 2013 02:43:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UVFtM-00089M-DR for qemu-devel@nongnu.org; Thu, 25 Apr 2013 02:42:57 -0400 Received: from e23smtp05.au.ibm.com ([202.81.31.147]:35902) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVFtL-00088c-JZ for qemu-devel@nongnu.org; Thu, 25 Apr 2013 02:42:52 -0400 Received: from /spool/local by e23smtp05.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 25 Apr 2013 16:27:39 +1000 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [9.190.235.152]) by d23dlp01.au.ibm.com (Postfix) with ESMTP id 307BE2CE804D for ; Thu, 25 Apr 2013 16:32:39 +1000 (EST) Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r3P6IrK460817484 for ; Thu, 25 Apr 2013 16:18:53 +1000 Received: from d23av01.au.ibm.com (loopback [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r3P6WbNB021900 for ; Thu, 25 Apr 2013 16:32:37 +1000 Message-ID: <5178CDB7.8080706@linux.vnet.ibm.com> Date: Thu, 25 Apr 2013 14:31:19 +0800 From: Wenchao Xia MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=GB2312 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Hrdina Cc: kwolf@redhat.com, lcapitulino@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com > Finding snapshot by a name which could also be an id isn't best way > how to do it. There will be rewrite of savevm, loadvm and delvm to > improve the behavior of these commands. The savevm and loadvm will > have their own patch series. > > Now bdrv_snapshot_find takes more parameters. The name parameter will > be matched only against the name of the snapshot and the same applies > to id parameter. > > There is one exception. If you set the last parameter, the name parameter > will be matched against the name or the id of a snapshot. This exception > is only for backward compatibility for other commands and it will be > dropped after all commands will be rewritten. > > We only need to know if that snapshot exists or not. We don't care > about any error message. If snapshot exists it returns TRUE otherwise > it returns FALSE. > > There is also new Error parameter which will containt error messeage if > something goes wrong. > > Signed-off-by: Pavel Hrdina > --- > savevm.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 67 insertions(+), 26 deletions(-) > > diff --git a/savevm.c b/savevm.c > index ba97c41..1622c55 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -2262,26 +2262,66 @@ out: > return ret; > } > > -static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, > - const char *name) > +static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, > + const char *name, const char *id, Error **errp, > + bool old_match) suggest directly drop old_match parameter here, or squash patch 12 into this one, mark the change in commit message, then the logic will be clearer. Personally hope to place parameter *id before *name. > { > QEMUSnapshotInfo *sn_tab, *sn; > - int nb_sns, i, ret; > + int nb_sns, i; > + bool found = false; > + > + assert(name || id); > > - ret = -ENOENT; > nb_sns = bdrv_snapshot_list(bs, &sn_tab); > - if (nb_sns < 0) > - return ret; > - for(i = 0; i < nb_sns; i++) { > + if (nb_sns < 0) { > + error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list"); > + return found; > + } > + > + if (nb_sns == 0) { > + error_setg(errp, "Device has no snapshots"); > + return found; > + } suggest not set errp here, which can be used to tip exception, but having not found one is a normal case. > + > + for (i = 0; i < nb_sns; i++) { > sn = &sn_tab[i]; > - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { > - *sn_info = *sn; > - ret = 0; > - break; > + if (name && id) { > + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) { > + *sn_info = *sn; > + found = true; > + break; > + } > + } else if (name) { > + /* for compatibility for old bdrv_snapshot_find call > + * will be removed */ > + if (old_match) { > + if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { > + *sn_info = *sn; > + found = true; > + break; > + } > + } else { > + if (!strcmp(sn->name, name)) { > + *sn_info = *sn; > + found = true; > + break; > + } > + } > + } else if (id) { > + if (!strcmp(sn->id_str, id)) { > + *sn_info = *sn; > + found = true; > + break; > + } > } > } suggest change the sequence: if (name && id) { for () { } } else if (name){ for () { } } else if (id) { for () { } } slightly faster, and make logic more clear. > + > + if (!found) { > + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id); suggest not to set error, since it is a normal case. > + } > + > g_free(sn_tab); > - return ret; > + return found; > } > > /* > @@ -2296,7 +2336,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name) > bs = NULL; > while ((bs = bdrv_next(bs))) { > if (bdrv_can_snapshot(bs) && > - bdrv_snapshot_find(bs, snapshot, name) >= 0) > + bdrv_snapshot_find(bs, snapshot, name, NULL, NULL, true)) > { > bdrv_snapshot_delete(bs, name, &local_err); > if (error_is_set(&local_err)) { > @@ -2358,8 +2398,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) > sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock); > > if (name) { > - ret = bdrv_snapshot_find(bs, old_sn, name); > - if (ret >= 0) { > + if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL, true)) { > pstrcpy(sn->name, sizeof(sn->name), old_sn->name); > pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str); > } else { > @@ -2440,6 +2479,7 @@ int load_vmstate(const char *name) > BlockDriverState *bs, *bs_vm_state; > QEMUSnapshotInfo sn; > QEMUFile *f; > + Error *local_err = NULL; > int ret; > > bs_vm_state = bdrv_snapshots(); > @@ -2449,9 +2489,10 @@ int load_vmstate(const char *name) > } > > /* Don't even try to load empty VM states */ > - ret = bdrv_snapshot_find(bs_vm_state, &sn, name); > - if (ret < 0) { > - return ret; > + if (!bdrv_snapshot_find(bs_vm_state, &sn, name, NULL, &local_err, true)) { > + error_report("Snapshot doesn't exist: %s", error_get_pretty(local_err)); > + error_free(local_err); > + return -ENOENT; > } else if (sn.vm_state_size == 0) { > error_report("This is a disk-only snapshot. Revert to it offline " > "using qemu-img."); > @@ -2473,11 +2514,11 @@ int load_vmstate(const char *name) > return -ENOTSUP; > } > > - ret = bdrv_snapshot_find(bs, &sn, name); > - if (ret < 0) { > - error_report("Device '%s' does not have the requested snapshot '%s'", > - bdrv_get_device_name(bs), name); > - return ret; > + if (!bdrv_snapshot_find(bs, &sn, name, NULL, &local_err, true)) { > + error_report("Snapshot doesn't exist for device '%s': %s", > + bdrv_get_device_name(bs), error_get_pretty(local_err)); > + error_free(local_err); > + return -ENOENT; > } > } > > @@ -2546,7 +2587,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) > { > BlockDriverState *bs, *bs1; > QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s; > - int nb_sns, i, ret, available; > + int nb_sns, i, available; > int total; > int *available_snapshots; > char buf[256]; > @@ -2577,8 +2618,8 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) > > while ((bs1 = bdrv_next(bs1))) { > if (bdrv_can_snapshot(bs1) && bs1 != bs) { > - ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str); > - if (ret < 0) { > + if (!bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL, NULL, > + true)) { > available = 0; > break; > } > -- Best Regards Wenchao Xia