| Message ID | bf08850cd61d5fb6b2ce3a0fee57b37336f36ac8.1366817130.git.phrdina@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/24/2013 09:32 AM, Pavel Hrdina wrote: > 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 double-typo and grammar: s/also/also a/ s/containt error messeage/contain the error message/ > something goes wrong. > > Signed-off-by: Pavel Hrdina <phrdina@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) I'd add a FIXME comment here documenting that you intend to remove the old_match parameter after all callers have been updated to the new semantics. > { > QEMUSnapshotInfo *sn_tab, *sn; > - int nb_sns, i, ret; > + int nb_sns, i; > + bool found = false; Bikeshedding: I don't think you need this variable, if you would instead do... > + > + 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; return false; > + } > + > + if (nb_sns == 0) { > + error_setg(errp, "Device has no snapshots"); > + return found; return false; > + } *sn_info = NULL; > + > + 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; Drop this assignment and others like it... > + 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) { use 'if (*sn_info)' > + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id); > + } > + > g_free(sn_tab); > - return ret; > + return found; return *sn_info != NULL; > } If you _do_ decide to keep the boolean variable instead of hard-coding a false return and avoiding redundancy by using other variables to determine the result, then at least s/found/ret/, because I find 'return found' as a way to intentionally fail rather odd-looking. At any rate, I can live with this logic, and all the conversions of existing call sites properly passed the given name, NULL id, and true for old_match semantics; along with optional deciding whether to pass NULL or a local error based on whether it would ignore lookup failure or propagate it as a failure of the higher-level operation that needed a lookup.
> 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 <phrdina@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) 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; > } >
On 24.4.2013 23:26, Eric Blake wrote: > On 04/24/2013 09:32 AM, Pavel Hrdina wrote: >> 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 > > double-typo and grammar: > s/also/also a/ > s/containt error messeage/contain the error message/ > >> something goes wrong. >> >> Signed-off-by: Pavel Hrdina <phrdina@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) > > I'd add a FIXME comment here documenting that you intend to remove the > old_match parameter after all callers have been updated to the new > semantics. > >> { >> QEMUSnapshotInfo *sn_tab, *sn; >> - int nb_sns, i, ret; >> + int nb_sns, i; >> + bool found = false; > > Bikeshedding: I don't think you need this variable, if you would instead > do... > >> + >> + 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; > > return false; > >> + } >> + >> + if (nb_sns == 0) { >> + error_setg(errp, "Device has no snapshots"); >> + return found; > > return false; > >> + } > > *sn_info = NULL; You cannot do that. It should be sn_info = NULL, but if you look at the usage of the bdrv_snapshot_find you will see that you cannot do that too. And the same applies ... > >> + >> + 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; > > Drop this assignment and others like it... > >> + 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) { > > use 'if (*sn_info)' to this usage and ... > >> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id); >> + } >> + >> g_free(sn_tab); >> - return ret; >> + return found; > > return *sn_info != NULL; this one. > >> } > > If you _do_ decide to keep the boolean variable instead of hard-coding a > false return and avoiding redundancy by using other variables to > determine the result, then at least s/found/ret/, because I find 'return > found' as a way to intentionally fail rather odd-looking. > > At any rate, I can live with this logic, and all the conversions of > existing call sites properly passed the given name, NULL id, and true > for old_match semantics; along with optional deciding whether to pass > NULL or a local error based on whether it would ignore lookup failure or > propagate it as a failure of the higher-level operation that needed a > lookup. > You are right that avoiding redundancy is better and I just think up a new solution. static QEMUSnapshotInfo *bdrv_snapshot_find(BlockDriverState *bs, const char *name, const char *id, Error **errp, bool old_match /*FIXME*/) And the bdrv_snapshot_find will return QEMUSnapshotInfo* on success and on error it will set the error message and also return NULL. Pavel
On 25.4.2013 08:31, Wenchao Xia wrote: >> 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 <phrdina@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) > 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. That make sense, I'll change it. > >> { >> 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. You don't have to use the error message. It is set as the error message but actually is more like an announcement. > >> + >> + 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. Thanks for suggest, I'll change the logic. > >> + >> + if (!found) { >> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id); > suggest not to set error, since it is a normal case. The same comment as for "Device has no snapshots". Pavel
On 25.4.2013 08:46, Pavel Hrdina wrote: > On 24.4.2013 23:26, Eric Blake wrote: >> On 04/24/2013 09:32 AM, Pavel Hrdina wrote: >>> 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 >> >> double-typo and grammar: >> s/also/also a/ >> s/containt error messeage/contain the error message/ >> >>> something goes wrong. >>> >>> Signed-off-by: Pavel Hrdina <phrdina@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) >> >> I'd add a FIXME comment here documenting that you intend to remove the >> old_match parameter after all callers have been updated to the new >> semantics. >> >>> { >>> QEMUSnapshotInfo *sn_tab, *sn; >>> - int nb_sns, i, ret; >>> + int nb_sns, i; >>> + bool found = false; >> >> Bikeshedding: I don't think you need this variable, if you would instead >> do... >> >>> + >>> + 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; >> >> return false; >> >>> + } >>> + >>> + if (nb_sns == 0) { >>> + error_setg(errp, "Device has no snapshots"); >>> + return found; >> >> return false; >> >>> + } >> >> *sn_info = NULL; > > You cannot do that. It should be sn_info = NULL, but if you look at the > usage of the bdrv_snapshot_find you will see that you cannot do that > too. And the same applies ... > >> >>> + >>> + 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; >> >> Drop this assignment and others like it... >> >>> + 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) { >> >> use 'if (*sn_info)' > > to this usage and ... > >> >>> + error_setg(errp, "Failed to find snapshot '%s'", name ? name >>> : id); >>> + } >>> + >>> g_free(sn_tab); >>> - return ret; >>> + return found; >> >> return *sn_info != NULL; > > this one. > >> >>> } >> >> If you _do_ decide to keep the boolean variable instead of hard-coding a >> false return and avoiding redundancy by using other variables to >> determine the result, then at least s/found/ret/, because I find 'return >> found' as a way to intentionally fail rather odd-looking. >> >> At any rate, I can live with this logic, and all the conversions of >> existing call sites properly passed the given name, NULL id, and true >> for old_match semantics; along with optional deciding whether to pass >> NULL or a local error based on whether it would ignore lookup failure or >> propagate it as a failure of the higher-level operation that needed a >> lookup. >> > > You are right that avoiding redundancy is better and I just think up a > new solution. > > static QEMUSnapshotInfo *bdrv_snapshot_find(BlockDriverState *bs, > const char *name, > const char *id, > Error **errp, > bool old_match /*FIXME*/) > > And the bdrv_snapshot_find will return QEMUSnapshotInfo* on success and > on error it will set the error message and also return NULL. > > Pavel > Well, this doesn't work too. I'll stick with the bool return value and the bool ret variable. Pavel
On 04/25/2013 12:31 AM, Wenchao Xia wrote: > >> + >> + if (!found) { >> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id); > suggest not to set error, since it is a normal case. The way I understand it, failure to find a snapshot might need to be treated as an error - it's up to the caller's needs. Also, there pretty much is only one failure mode - the requested snapshot was not found - even if there are multiple ways that we can fail to find a requested snapshot, so I'm fine with treating all 'false' returns as an error path. Thus, a caller that wants to probe for a snapshot existence but not set an error calls: bdrv_snapshot_find(bs, snapshot, name, id, NULL, false); while a caller that wants to report a missing snapshot as an error calls: bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false); and then propagates local_err on upwards. Or are you worried about a possible third case, where a caller cares about failure during bdrv_snapshot_list(), differently than failure to find a snapshot? What callers have that semantics? If that is a real concern, then maybe returning a bool is the wrong approach, and we should instead return an int. A return < 0 is a fatal error (bdrv_snapshot_list failed to even look up snapshots); a return of 0 means our lookup attempt hit no fatal errors but the snapshot was not found, and a return of 1 means the snapshot was found. Then there would be three calling styles: Probe for existence, with no error reporting: if (bdrv_snapshot_find(bs, snapshot, name, id, NULL, false) > 0) { // exists } Probe for existence but with error reporting on fatal errors: exist = bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false); if (exist < 0) { // propagate local_err } else if (exist) { // exists } Probe for snapshot, with error reporting even for failed lookup: if (bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false) <= 0) { // propagate local_err } But I don't know what the existing callers need, to make a decision on whether a signature change is warranted. Again, more reason to defer this series to 1.6.
δΊ 2013-4-25 20:16, Eric Blake ει: > On 04/25/2013 12:31 AM, Wenchao Xia wrote: >> >>> + >>> + if (!found) { >>> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id); >> suggest not to set error, since it is a normal case. > > The way I understand it, failure to find a snapshot might need to be > treated as an error - it's up to the caller's needs. Also, there pretty > much is only one failure mode - the requested snapshot was not found - > even if there are multiple ways that we can fail to find a requested > snapshot, so I'm fine with treating all 'false' returns as an error path. > > Thus, a caller that wants to probe for a snapshot existence but not set > an error calls: > bdrv_snapshot_find(bs, snapshot, name, id, NULL, false); > > while a caller that wants to report a missing snapshot as an error calls: > bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false); > and then propagates local_err on upwards. > > > Or are you worried about a possible third case, where a caller cares > about failure during bdrv_snapshot_list(), differently than failure to > find a snapshot? What callers have that semantics? If that is a real > concern, then maybe returning a bool is the wrong approach, and we > should instead return an int. A return < 0 is a fatal error > (bdrv_snapshot_list failed to even look up snapshots); a return of 0 > means our lookup attempt hit no fatal errors but the snapshot was not > found, and a return of 1 means the snapshot was found. Then there would > be three calling styles: > > Probe for existence, with no error reporting: > if (bdrv_snapshot_find(bs, snapshot, name, id, NULL, false) > 0) { > // exists > } > Probe for existence but with error reporting on fatal errors: > exist = bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false); > if (exist < 0) { > // propagate local_err > } else if (exist) { > // exists > } > Probe for snapshot, with error reporting even for failed lookup: > if (bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false) <= 0) { > // propagate local_err > } > > But I don't know what the existing callers need, to make a decision on > whether a signature change is warranted. Again, more reason to defer > this series to 1.6. > Personally I prefer internal layer have clean meaning, setting error only for exception. But I am not strongly against it, if caller can make easier use of it, a document for this function is also OK.
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 <phrdina@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
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"); + 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); + } + 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; }
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 <phrdina@redhat.com> --- savevm.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 67 insertions(+), 26 deletions(-)