Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter

[Eric Blake]

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 <address@hidden>
> ---
>  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