Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben: > 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 <phrd...@redhat.com> > --- > 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) > { > 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");
This is not an error case, please remove the error_setg(). If the caller indeed needs to have a snapshot, it can generate an error by himself. We cannot assume here that every caller needs it. > + return found; > + } > + > + 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; > + } > } > } > + > + if (!found) { > + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id); > + } Same here. > + > 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)); "Could not find snapshot" (it could just be an I/O error in reading the snapshot list) Or actually, this message is fine for the case where you don't find the snapshot, but have no error in bdrv_snapshot_find(). Should the device name of bs_vm_state be mentioned in the error message? > + 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."); Kevin