Patchwork [v2,03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter

login
register
mail settings
Submitter Pavel Hrdina
Date April 24, 2013, 3:32 p.m.
Message ID <bf08850cd61d5fb6b2ce3a0fee57b37336f36ac8.1366817130.git.phrdina@redhat.com>
Download mbox | patch
Permalink /patch/239232/
State New
Headers show

Comments

Pavel Hrdina - April 24, 2013, 3:32 p.m.
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(-)
Eric Blake - April 24, 2013, 9:26 p.m.
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.
Wayne Xia - April 25, 2013, 6:31 a.m.
> 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;
>                   }
>
Pavel Hrdina - April 25, 2013, 6:46 a.m.
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
Pavel Hrdina - April 25, 2013, 6:52 a.m.
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
Pavel Hrdina - April 25, 2013, 8:18 a.m.
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
Eric Blake - April 25, 2013, 12:16 p.m.
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.
Wayne Xia - April 26, 2013, 2:37 a.m.
于 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.
Kevin Wolf - May 3, 2013, 10:24 a.m.
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

Patch

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;
                 }