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

[Pavel Hrdina <phrdina@redhat.com>]

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