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 <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)
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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
