Patchwork [11/11] savevm: remove backward compatibility from bdrv_snapshot_find()

login
register
mail settings
Submitter Pavel Hrdina
Date April 16, 2013, 4:05 p.m.
Message ID <13b9d1e79947b89982ec51c421b9b1bd0a7b587d.1366127809.git.phrdina@redhat.com>
Download mbox | patch
Permalink /patch/237023/
State New
Headers show

Comments

Pavel Hrdina - April 16, 2013, 4:05 p.m.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 savevm.c | 33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)
Wayne Xia - April 17, 2013, 2:53 a.m.
Hi, Pavel
  I have implemented it at
http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg02533.html
in patch 1,2. Could u check if it satisfy your requirement, if yes maybe
you can directly use them.

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>   savevm.c | 33 +++++++++++----------------------
>   1 file changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 66753da..bc829a5 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2195,7 +2195,7 @@ out:
>   }
> 
>   static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> -                              const char *name, const char *id, bool old_match)
> +                              const char *name, const char *id)
>   {
>       QEMUSnapshotInfo *sn_tab, *sn;
>       int nb_sns, i, found = 0;
> @@ -2218,20 +2218,10 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>                   break;
>               }
>           } else if (name) {
> -            /* for compatibility for old bdrv_snapshot_find call
> -             * will be removed */
> -            if (old_match) {
> -                if (!strcmp(sn->id_str, id) || !strcmp(sn->name, name)) {
> -                    *sn_info = *sn;
> -                    found = 1;
> -                    break;
> -                }
> -            } else {
> -                if (!strcmp(sn->name, name)) {
> -                    *sn_info = *sn;
> -                    found = 1;
> -                    break;
> -                }
> +            if (!strcmp(sn->name, name)) {
> +                *sn_info = *sn;
> +                found = 1;
> +                break;
>               }
>           } else if (id) {
>               if (!strcmp(sn->id_str, id)) {
> @@ -2290,7 +2280,7 @@ SnapshotInfo *qmp_vm_snapshot_save(const char *name, Error **errp)
>       sn->date_nsec = tv.tv_usec * 1000;
>       sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
> 
> -    if (bdrv_snapshot_find(bs, old_sn, name, NULL, false)) {
> +    if (bdrv_snapshot_find(bs, old_sn, name, NULL)) {
>           error_setg(errp, "snapshot '%s' exists", name);
>           goto the_end;
>       } else {
> @@ -2388,7 +2378,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
>       }
> 
>       /* Don't even try to load empty VM states */
> -    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, false)) {
> +    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id)) {
>           return NULL;
>       }
> 
> @@ -2413,7 +2403,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
>               return NULL;
>           }
> 
> -        if (!bdrv_snapshot_find(bs, &sn, name, id, false)) {
> +        if (!bdrv_snapshot_find(bs, &sn, name, id)) {
>               return NULL;
>           }
>       }
> @@ -2484,7 +2474,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
>           return NULL;
>       }
> 
> -    if (!bdrv_snapshot_find(bs, &sn, name, id, false)) {
> +    if (!bdrv_snapshot_find(bs, &sn, name, id)) {
>           /* no need to set an error if snapshot doesn't exist */
>           return NULL;
>       }
> @@ -2501,7 +2491,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
>       bs = NULL;
>       while ((bs = bdrv_next(bs))) {
>           if (bdrv_can_snapshot(bs)
> -                && bdrv_snapshot_find(bs, &sn, name, id, false)) {
> +                && bdrv_snapshot_find(bs, &sn, name, id)) {
>               bdrv_snapshot_delete(bs, sn.name, errp);
>               if (error_is_set(errp)) {
>                   return NULL;
> @@ -2549,8 +2539,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
> 
>           while ((bs1 = bdrv_next(bs1))) {
>               if (bdrv_can_snapshot(bs1) && bs1 != bs) {
> -                if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str,
> -                                        true)) {
> +                if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str)) {
>                       available = 0;
>                       break;
>                   }
>
Pavel Hrdina - April 17, 2013, 7:52 a.m.
Hi Wenchao,

unfortunately no. According to new design of savevm, loadvm and delvm I
need also search for snapshots that have the specified name and id.

I'm also touching bdrv_snapshot_list where I'm adding an Error parameter
and changing the return value to be used only for getting a number of
snapshots. So in case that there is some error, the return value will be 0.

Pavel

On 17.4.2013 04:53, Wenchao Xia wrote:
> Hi, Pavel
>    I have implemented it at
> http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg02533.html
> in patch 1,2. Could u check if it satisfy your requirement, if yes maybe
> you can directly use them.
> 
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>>    savevm.c | 33 +++++++++++----------------------
>>    1 file changed, 11 insertions(+), 22 deletions(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index 66753da..bc829a5 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2195,7 +2195,7 @@ out:
>>    }
>>
>>    static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>> -                              const char *name, const char *id, bool old_match)
>> +                              const char *name, const char *id)
>>    {
>>        QEMUSnapshotInfo *sn_tab, *sn;
>>        int nb_sns, i, found = 0;
>> @@ -2218,20 +2218,10 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>                    break;
>>                }
>>            } else if (name) {
>> -            /* for compatibility for old bdrv_snapshot_find call
>> -             * will be removed */
>> -            if (old_match) {
>> -                if (!strcmp(sn->id_str, id) || !strcmp(sn->name, name)) {
>> -                    *sn_info = *sn;
>> -                    found = 1;
>> -                    break;
>> -                }
>> -            } else {
>> -                if (!strcmp(sn->name, name)) {
>> -                    *sn_info = *sn;
>> -                    found = 1;
>> -                    break;
>> -                }
>> +            if (!strcmp(sn->name, name)) {
>> +                *sn_info = *sn;
>> +                found = 1;
>> +                break;
>>                }
>>            } else if (id) {
>>                if (!strcmp(sn->id_str, id)) {
>> @@ -2290,7 +2280,7 @@ SnapshotInfo *qmp_vm_snapshot_save(const char *name, Error **errp)
>>        sn->date_nsec = tv.tv_usec * 1000;
>>        sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
>>
>> -    if (bdrv_snapshot_find(bs, old_sn, name, NULL, false)) {
>> +    if (bdrv_snapshot_find(bs, old_sn, name, NULL)) {
>>            error_setg(errp, "snapshot '%s' exists", name);
>>            goto the_end;
>>        } else {
>> @@ -2388,7 +2378,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
>>        }
>>
>>        /* Don't even try to load empty VM states */
>> -    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, false)) {
>> +    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id)) {
>>            return NULL;
>>        }
>>
>> @@ -2413,7 +2403,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
>>                return NULL;
>>            }
>>
>> -        if (!bdrv_snapshot_find(bs, &sn, name, id, false)) {
>> +        if (!bdrv_snapshot_find(bs, &sn, name, id)) {
>>                return NULL;
>>            }
>>        }
>> @@ -2484,7 +2474,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
>>            return NULL;
>>        }
>>
>> -    if (!bdrv_snapshot_find(bs, &sn, name, id, false)) {
>> +    if (!bdrv_snapshot_find(bs, &sn, name, id)) {
>>            /* no need to set an error if snapshot doesn't exist */
>>            return NULL;
>>        }
>> @@ -2501,7 +2491,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
>>        bs = NULL;
>>        while ((bs = bdrv_next(bs))) {
>>            if (bdrv_can_snapshot(bs)
>> -                && bdrv_snapshot_find(bs, &sn, name, id, false)) {
>> +                && bdrv_snapshot_find(bs, &sn, name, id)) {
>>                bdrv_snapshot_delete(bs, sn.name, errp);
>>                if (error_is_set(errp)) {
>>                    return NULL;
>> @@ -2549,8 +2539,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
>>
>>            while ((bs1 = bdrv_next(bs1))) {
>>                if (bdrv_can_snapshot(bs1) && bs1 != bs) {
>> -                if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str,
>> -                                        true)) {
>> +                if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str)) {
>>                        available = 0;
>>                        break;
>>                    }
>>
> 
>
Wayne Xia - April 17, 2013, 10:19 a.m.
于 2013-4-17 15:52, Pavel Hrdina 写道:
> Hi Wenchao,
> 
> unfortunately no. According to new design of savevm, loadvm and delvm I
> need also search for snapshots that have the specified name and id.
> 
  It seems the logic in your function, is same with mine...

> I'm also touching bdrv_snapshot_list where I'm adding an Error parameter
  I looked it before, but it needs all call back in ./block support it,
so is it really necessary?

> and changing the return value to be used only for getting a number of
> snapshots. So in case that there is some error, the return value will be 0.
> 
> Pavel
> 
> On 17.4.2013 04:53, Wenchao Xia wrote:
>> Hi, Pavel
>>     I have implemented it at
>> http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg02533.html
>> in patch 1,2. Could u check if it satisfy your requirement, if yes maybe
>> you can directly use them.
>>
>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>> ---
>>>     savevm.c | 33 +++++++++++----------------------
>>>     1 file changed, 11 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/savevm.c b/savevm.c
>>> index 66753da..bc829a5 100644
>>> --- a/savevm.c
>>> +++ b/savevm.c
>>> @@ -2195,7 +2195,7 @@ out:
>>>     }
>>>
>>>     static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>> -                              const char *name, const char *id, bool old_match)
>>> +                              const char *name, const char *id)
>>>     {
>>>         QEMUSnapshotInfo *sn_tab, *sn;
>>>         int nb_sns, i, found = 0;
>>> @@ -2218,20 +2218,10 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>>                     break;
>>>                 }
>>>             } else if (name) {
>>> -            /* for compatibility for old bdrv_snapshot_find call
>>> -             * will be removed */
>>> -            if (old_match) {
>>> -                if (!strcmp(sn->id_str, id) || !strcmp(sn->name, name)) {
>>> -                    *sn_info = *sn;
>>> -                    found = 1;
>>> -                    break;
>>> -                }
>>> -            } else {
>>> -                if (!strcmp(sn->name, name)) {
>>> -                    *sn_info = *sn;
>>> -                    found = 1;
>>> -                    break;
>>> -                }
>>> +            if (!strcmp(sn->name, name)) {
>>> +                *sn_info = *sn;
>>> +                found = 1;
>>> +                break;
>>>                 }
>>>             } else if (id) {
>>>                 if (!strcmp(sn->id_str, id)) {
>>> @@ -2290,7 +2280,7 @@ SnapshotInfo *qmp_vm_snapshot_save(const char *name, Error **errp)
>>>         sn->date_nsec = tv.tv_usec * 1000;
>>>         sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
>>>
>>> -    if (bdrv_snapshot_find(bs, old_sn, name, NULL, false)) {
>>> +    if (bdrv_snapshot_find(bs, old_sn, name, NULL)) {
>>>             error_setg(errp, "snapshot '%s' exists", name);
>>>             goto the_end;
>>>         } else {
>>> @@ -2388,7 +2378,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
>>>         }
>>>
>>>         /* Don't even try to load empty VM states */
>>> -    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, false)) {
>>> +    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id)) {
>>>             return NULL;
>>>         }
>>>
>>> @@ -2413,7 +2403,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
>>>                 return NULL;
>>>             }
>>>
>>> -        if (!bdrv_snapshot_find(bs, &sn, name, id, false)) {
>>> +        if (!bdrv_snapshot_find(bs, &sn, name, id)) {
>>>                 return NULL;
>>>             }
>>>         }
>>> @@ -2484,7 +2474,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
>>>             return NULL;
>>>         }
>>>
>>> -    if (!bdrv_snapshot_find(bs, &sn, name, id, false)) {
>>> +    if (!bdrv_snapshot_find(bs, &sn, name, id)) {
>>>             /* no need to set an error if snapshot doesn't exist */
>>>             return NULL;
>>>         }
>>> @@ -2501,7 +2491,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
>>>         bs = NULL;
>>>         while ((bs = bdrv_next(bs))) {
>>>             if (bdrv_can_snapshot(bs)
>>> -                && bdrv_snapshot_find(bs, &sn, name, id, false)) {
>>> +                && bdrv_snapshot_find(bs, &sn, name, id)) {
>>>                 bdrv_snapshot_delete(bs, sn.name, errp);
>>>                 if (error_is_set(errp)) {
>>>                     return NULL;
>>> @@ -2549,8 +2539,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
>>>
>>>             while ((bs1 = bdrv_next(bs1))) {
>>>                 if (bdrv_can_snapshot(bs1) && bs1 != bs) {
>>> -                if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str,
>>> -                                        true)) {
>>> +                if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str)) {
>>>                         available = 0;
>>>                         break;
>>>                     }
>>>
>>
>>
> 
>
Pavel Hrdina - April 17, 2013, 10:51 a.m.
On 17.4.2013 12:19, Wenchao Xia wrote:
> 于 2013-4-17 15:52, Pavel Hrdina 写道:
>> Hi Wenchao,
>>
>> unfortunately no. According to new design of savevm, loadvm and delvm I
>> need also search for snapshots that have the specified name and id.
>>
>    It seems the logic in your function, is same with mine...

It is not the same.

Your logic:
    if id is set:
        if there is snapshot with that id:
            end searching
    if name set (search also if id is set but nothing found):
        if there is snapshot with that name:
            end searching

My logic:
    if name is set and id is set:
        if there is snapshot with than name and with that id:
            end searching
    else if name is set (means that only name is set):
        if there is snapshot with that name:
            end searching
    else if id is set (means that only id is set):
        if there is snapshot with that id:
            end searching

> 
>> I'm also touching bdrv_snapshot_list where I'm adding an Error parameter
>    I looked it before, but it needs all call back in ./block support it,
> so is it really necessary?

I think it is better if this function internally set appropriate error
message based on used disk image format (qcow2, sheepdog, rbd).

Adding to CC Eric for his opinion.

> 
>> and changing the return value to be used only for getting a number of
>> snapshots. So in case that there is some error, the return value will be 0.
>>
>> Pavel
>>
>> On 17.4.2013 04:53, Wenchao Xia wrote:
>>> Hi, Pavel
>>>      I have implemented it at
>>> http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg02533.html
>>> in patch 1,2. Could u check if it satisfy your requirement, if yes maybe
>>> you can directly use them.
>>>
>>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>>> ---
>>>>      savevm.c | 33 +++++++++++----------------------
>>>>      1 file changed, 11 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/savevm.c b/savevm.c
>>>> index 66753da..bc829a5 100644
>>>> --- a/savevm.c
>>>> +++ b/savevm.c
>>>> @@ -2195,7 +2195,7 @@ out:
>>>>      }
>>>>
>>>>      static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>>> -                              const char *name, const char *id, bool old_match)
>>>> +                              const char *name, const char *id)
>>>>      {
>>>>          QEMUSnapshotInfo *sn_tab, *sn;
>>>>          int nb_sns, i, found = 0;
>>>> @@ -2218,20 +2218,10 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>>>                      break;
>>>>                  }
>>>>              } else if (name) {
>>>> -            /* for compatibility for old bdrv_snapshot_find call
>>>> -             * will be removed */
>>>> -            if (old_match) {
>>>> -                if (!strcmp(sn->id_str, id) || !strcmp(sn->name, name)) {
>>>> -                    *sn_info = *sn;
>>>> -                    found = 1;
>>>> -                    break;
>>>> -                }
>>>> -            } else {
>>>> -                if (!strcmp(sn->name, name)) {
>>>> -                    *sn_info = *sn;
>>>> -                    found = 1;
>>>> -                    break;
>>>> -                }
>>>> +            if (!strcmp(sn->name, name)) {
>>>> +                *sn_info = *sn;
>>>> +                found = 1;
>>>> +                break;
>>>>                  }
>>>>              } else if (id) {
>>>>                  if (!strcmp(sn->id_str, id)) {
>>>> @@ -2290,7 +2280,7 @@ SnapshotInfo *qmp_vm_snapshot_save(const char *name, Error **errp)
>>>>          sn->date_nsec = tv.tv_usec * 1000;
>>>>          sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
>>>>
>>>> -    if (bdrv_snapshot_find(bs, old_sn, name, NULL, false)) {
>>>> +    if (bdrv_snapshot_find(bs, old_sn, name, NULL)) {
>>>>              error_setg(errp, "snapshot '%s' exists", name);
>>>>              goto the_end;
>>>>          } else {
>>>> @@ -2388,7 +2378,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
>>>>          }
>>>>
>>>>          /* Don't even try to load empty VM states */
>>>> -    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, false)) {
>>>> +    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id)) {
>>>>              return NULL;
>>>>          }
>>>>
>>>> @@ -2413,7 +2403,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
>>>>                  return NULL;
>>>>              }
>>>>
>>>> -        if (!bdrv_snapshot_find(bs, &sn, name, id, false)) {
>>>> +        if (!bdrv_snapshot_find(bs, &sn, name, id)) {
>>>>                  return NULL;
>>>>              }
>>>>          }
>>>> @@ -2484,7 +2474,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
>>>>              return NULL;
>>>>          }
>>>>
>>>> -    if (!bdrv_snapshot_find(bs, &sn, name, id, false)) {
>>>> +    if (!bdrv_snapshot_find(bs, &sn, name, id)) {
>>>>              /* no need to set an error if snapshot doesn't exist */
>>>>              return NULL;
>>>>          }
>>>> @@ -2501,7 +2491,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
>>>>          bs = NULL;
>>>>          while ((bs = bdrv_next(bs))) {
>>>>              if (bdrv_can_snapshot(bs)
>>>> -                && bdrv_snapshot_find(bs, &sn, name, id, false)) {
>>>> +                && bdrv_snapshot_find(bs, &sn, name, id)) {
>>>>                  bdrv_snapshot_delete(bs, sn.name, errp);
>>>>                  if (error_is_set(errp)) {
>>>>                      return NULL;
>>>> @@ -2549,8 +2539,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
>>>>
>>>>              while ((bs1 = bdrv_next(bs1))) {
>>>>                  if (bdrv_can_snapshot(bs1) && bs1 != bs) {
>>>> -                if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str,
>>>> -                                        true)) {
>>>> +                if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str)) {
>>>>                          available = 0;
>>>>                          break;
>>>>                      }
>>>>
>>>
>>>
>>
>>
> 
>
Eric Blake - April 17, 2013, 6:14 p.m.
On 04/17/2013 04:51 AM, Pavel Hrdina wrote:
> On 17.4.2013 12:19, Wenchao Xia wrote:
>> 于 2013-4-17 15:52, Pavel Hrdina 写道:
>>> Hi Wenchao,
>>>
>>> unfortunately no. According to new design of savevm, loadvm and delvm I
>>> need also search for snapshots that have the specified name and id.
>>>
>>    It seems the logic in your function, is same with mine...
> 
> It is not the same.

Consider a qcow2 file with the following snapshots:

     id     tag
     1      2
     2      1
     3      none

Existing logic:

only one string is passed, and only one loop is performed.  If it
matches id or name, end searching

search for 1 - finds id 1 tag 2
search for 2 - finds id 1 tag 2
search for 3 - finds id 3 no tag
search for 4 - finds nothing

no way to find id 2 tag 1

The last point proves that the current algorithm is not ideal, and that
we are justified in changing it.  But there are several ways to change
it, and a consideration of whether we should preserve backwards
compatibility in HMP by making the search itself have backwards
compatibility, or by making the QMP search follow strict rules where HMP
can emulate some measure of back-compat by calling into QMP more than once.

> 
> Your logic:
[Wenchao]
>     if id is set:
>         if there is snapshot with that id:
>             end searching
>     if name set (search also if id is set but nothing found):
>         if there is snapshot with that name:
>             end searching
> 
> My logic:
[Pavel]
>     if name is set and id is set:
>         if there is snapshot with than name and with that id:
>             end searching
>     else if name is set (means that only name is set):
>         if there is snapshot with that name:
>             end searching
>     else if id is set (means that only id is set):
>         if there is snapshot with that id:
>             end searching

Best is a side-by-side comparison; when comparing to existing, I showed
three different choices of expanding a single name into a two-argument
find call.

 search for:            finds                compared to
  id     name     Wenchao       Pavel          existing
                                              name -> find(id, NULL)
  1      NULL    id 1 tag 2    id 1 tag 2    id 1 tag 2
* 2      NULL    id 2 tag 1    id 2 tag 1    id 1 tag 2
  3      NULL    id 3 no tag   id 3 no tag   id 3 no tag
  4      NULL    nothing       nothing       nothing
                                              name -> find(NULL, tag)
* NULL   1       id 2 tag 1    id 2 tag 1    id 1 tag 2
  NULL   2       id 1 tag 2    id 1 tag 2    id 1 tag 2
* NULL   3       nothing       nothing       id 3 no tag
  NULL   4       nothing       nothing       nothing
                                              not possible
  1      2       id 1 tag 2    id 1 tag 2    --
  2      1       id 2 tag 1    id 2 tag 1    --
                                              name -> find(id, tag)
* 1      1       id 1 tag 2    nothing       id 1 tag 2
* 2      2       id 2 tag 1    nothing       id 1 tag 2
* 3      3       id 3 no tag   nothing       id 3 no tag
  4      4       nothing       nothing       nothing

The two proposed approaches both allow access to a lookup that the
original could not provide (namely, id 2 tag 1), so they are an
improvement on that front.  But the two approaches differ on behavior
when both id and name are specified (Wenchao's behaves the same as an
id-only lookup, regardless of whether the name matches; Pavel's requires
a double match).  The other thing to note is that the old code allowed a
single name to match an anonymous snapshot, but both proposals fail to
find a nameless snapshot without an id search.

Can I put yet another proposed algorithm on the table?  First, written
with four loops over the source (although at most two are taken):

    if name is set and id is set:
        if there is snapshot with than name and with that id (loop 1):
            end searching
    else if name is set (means that only name is set):
        if there is snapshot with that name (loop 2):
            end searching
        if there is snapshot with that id (loop 3):
            end searching
    else if id is set (means that only id is set):
        if there is snapshot with that id (loop 4):
            end searching

Or, written another way, to implement the same results in only two coded
loops:

    if name is set:
        if there is a snapshot with that name (loop 1):
            if id is set:
                if id matches:
                    end searching successfully
                else:
                    fail
            else:
                end searching successfully
        else if id is not set:
            set id to name
    if there is a snapshot with id (loop 2):
        end searching successfully

And the resulting comparison table, again with three possibilities of
how to convert one-argument lookup into two-argument:

 search for:        finds      compared to
  id     name     mine          existing
                                name -> find(id, NULL)
  1      NULL    id 1 tag 2    id 1 tag 2
* 2      NULL    id 2 tag 1    id 1 tag 2
  3      NULL    id 3 no tag   id 3 no tag
  4      NULL    nothing       nothing
                                name -> find(NULL, tag)
* NULL   1       id 2 tag 1    id 1 tag 2
  NULL   2       id 1 tag 2    id 1 tag 2
  NULL   3       id 3 no tag   id 3 no tag
  NULL   4       nothing       nothing
                                not possible
  1      2       id 1 tag 2    --
  2      1       id 2 tag 1    --
                                name -> find(id, tag)
* 1      1       nothing       id 1 tag 2
* 2      2       nothing       id 1 tag 2
* 3      3       nothing       id 3 no tag
  4      4       nothing       nothing


With that adjusted table, I would favor converting any single name
lookup into find(NULL, tag).  Only the new QMP command that lets us do
an explicit id lookup can search for an id regardless of another name
matching the id; but we at least have the benefit of being able to
access ALL named snapshots (better than the original code unable to
access tag 2), as well as the ability to access unambiguous ids without
extra effort (ability to access id 3 without having to change the
command line).  It keeps the aspect of Pavel's code that specifying both
fields must match both fields, but otherwise favors names over ids
(generally good, since names are generally not numeric, except when we
are talking about corner cases).

So, was I convincing enough in arguing that we want something different
from the approach of both existing patch series?

> 
>>
>>> I'm also touching bdrv_snapshot_list where I'm adding an Error parameter
>>    I looked it before, but it needs all call back in ./block support it,
>> so is it really necessary?
> 
> I think it is better if this function internally set appropriate error
> message based on used disk image format (qcow2, sheepdog, rbd).

I like the idea of find() setting errp on lookup failure.  Callers can
ignore errors in situations where a failed lookup triggers a reasonable
fallback, but in case the caller wants to report an error, the lower
down that we generate an error message, the more likely the error
message is to contain full context rather than being dumbed down by
generating it higher on the call stack.
Eric Blake - April 17, 2013, 6:22 p.m.
On 04/17/2013 12:14 PM, Eric Blake wrote:

> Or, written another way, to implement the same results in only two coded
> loops:

Missed a line:

> 
>     if name is set:
>         if there is a snapshot with that name (loop 1):
>             if id is set:
>                 if id matches:
>                     end searching successfully
>                 else:
>                     fail
>             else:
>                 end searching successfully
>         else if id is not set:
>             set id to name

These two lines should instead be:

          else if id is set:
              fail
          set id to name

>     if there is a snapshot with id (loop 2):
>         end searching successfully

and of course, all of the algorithms have an implied fail if you make it
through all of the loops without a successful search end.

The tweak above is needed so that a find(id, tag) that fails a tag
lookup does not then succeed on an id lookup.
Wayne Xia - April 18, 2013, 4:31 a.m.
于 2013-4-18 2:14, Eric Blake 写道:
> On 04/17/2013 04:51 AM, Pavel Hrdina wrote:
>> On 17.4.2013 12:19, Wenchao Xia wrote:
>>> 于 2013-4-17 15:52, Pavel Hrdina 写道:
>>>> Hi Wenchao,
>>>>
>>>> unfortunately no. According to new design of savevm, loadvm and delvm I
>>>> need also search for snapshots that have the specified name and id.
>>>>
>>>     It seems the logic in your function, is same with mine...
>>
>> It is not the same.
>
> Consider a qcow2 file with the following snapshots:
>
>       id     tag
>       1      2
>       2      1
>       3      none
>
> Existing logic:
>
> only one string is passed, and only one loop is performed.  If it
> matches id or name, end searching
>
> search for 1 - finds id 1 tag 2
> search for 2 - finds id 1 tag 2
> search for 3 - finds id 3 no tag
> search for 4 - finds nothing
>
> no way to find id 2 tag 1
>
> The last point proves that the current algorithm is not ideal, and that
> we are justified in changing it.  But there are several ways to change
> it, and a consideration of whether we should preserve backwards
> compatibility in HMP by making the search itself have backwards
> compatibility, or by making the QMP search follow strict rules where HMP
> can emulate some measure of back-compat by calling into QMP more than once.
>
>>
>> Your logic:
> [Wenchao]
>>      if id is set:
>>          if there is snapshot with that id:
>>              end searching
>>      if name set (search also if id is set but nothing found):
>>          if there is snapshot with that name:
>>              end searching
>>
>> My logic:
> [Pavel]
>>      if name is set and id is set:
>>          if there is snapshot with than name and with that id:
>>              end searching
>>      else if name is set (means that only name is set):
>>          if there is snapshot with that name:
>>              end searching
>>      else if id is set (means that only id is set):
>>          if there is snapshot with that id:
>>              end searching
>
> Best is a side-by-side comparison; when comparing to existing, I showed
> three different choices of expanding a single name into a two-argument
> find call.
>
>   search for:            finds                compared to
>    id     name     Wenchao       Pavel          existing
>                                                name -> find(id, NULL)
>    1      NULL    id 1 tag 2    id 1 tag 2    id 1 tag 2
> * 2      NULL    id 2 tag 1    id 2 tag 1    id 1 tag 2
>    3      NULL    id 3 no tag   id 3 no tag   id 3 no tag
>    4      NULL    nothing       nothing       nothing
>                                                name -> find(NULL, tag)
> * NULL   1       id 2 tag 1    id 2 tag 1    id 1 tag 2
>    NULL   2       id 1 tag 2    id 1 tag 2    id 1 tag 2
> * NULL   3       nothing       nothing       id 3 no tag
>    NULL   4       nothing       nothing       nothing
>                                                not possible
>    1      2       id 1 tag 2    id 1 tag 2    --
>    2      1       id 2 tag 1    id 2 tag 1    --
>                                                name -> find(id, tag)
> * 1      1       id 1 tag 2    nothing       id 1 tag 2
> * 2      2       id 2 tag 1    nothing       id 1 tag 2
> * 3      3       id 3 no tag   nothing       id 3 no tag
>    4      4       nothing       nothing       nothing
>
> The two proposed approaches both allow access to a lookup that the
> original could not provide (namely, id 2 tag 1), so they are an
> improvement on that front.  But the two approaches differ on behavior
> when both id and name are specified (Wenchao's behaves the same as an
> id-only lookup, regardless of whether the name matches; Pavel's requires
> a double match).  The other thing to note is that the old code allowed a
> single name to match an anonymous snapshot, but both proposals fail to
> find a nameless snapshot without an id search.
>
> Can I put yet another proposed algorithm on the table?  First, written
> with four loops over the source (although at most two are taken):
>
>      if name is set and id is set:
>          if there is snapshot with than name and with that id (loop 1):
>              end searching
>      else if name is set (means that only name is set):
>          if there is snapshot with that name (loop 2):
>              end searching
>          if there is snapshot with that id (loop 3):
>              end searching
>      else if id is set (means that only id is set):
>          if there is snapshot with that id (loop 4):
>              end searching
>
> Or, written another way, to implement the same results in only two coded
> loops:
>
>      if name is set:
>          if there is a snapshot with that name (loop 1):
>              if id is set:
>                  if id matches:
>                      end searching successfully
>                  else:
>                      fail
>              else:
>                  end searching successfully
>          else if id is not set:
>              set id to name
>      if there is a snapshot with id (loop 2):
>          end searching successfully
>
> And the resulting comparison table, again with three possibilities of
> how to convert one-argument lookup into two-argument:
>
>   search for:        finds      compared to
>    id     name     mine          existing
>                                  name -> find(id, NULL)
>    1      NULL    id 1 tag 2    id 1 tag 2
> * 2      NULL    id 2 tag 1    id 1 tag 2
>    3      NULL    id 3 no tag   id 3 no tag
>    4      NULL    nothing       nothing
>                                  name -> find(NULL, tag)
> * NULL   1       id 2 tag 1    id 1 tag 2
>    NULL   2       id 1 tag 2    id 1 tag 2
>    NULL   3       id 3 no tag   id 3 no tag
>    NULL   4       nothing       nothing
>                                  not possible
>    1      2       id 1 tag 2    --
>    2      1       id 2 tag 1    --
>                                  name -> find(id, tag)
> * 1      1       nothing       id 1 tag 2
> * 2      2       nothing       id 1 tag 2
> * 3      3       nothing       id 3 no tag
>    4      4       nothing       nothing
>
>
> With that adjusted table, I would favor converting any single name
> lookup into find(NULL, tag).  Only the new QMP command that lets us do
> an explicit id lookup can search for an id regardless of another name
> matching the id; but we at least have the benefit of being able to
> access ALL named snapshots (better than the original code unable to
> access tag 2), as well as the ability to access unambiguous ids without
> extra effort (ability to access id 3 without having to change the
> command line).  It keeps the aspect of Pavel's code that specifying both
> fields must match both fields, but otherwise favors names over ids
> (generally good, since names are generally not numeric, except when we
> are talking about corner cases).
>
> So, was I convincing enough in arguing that we want something different
> from the approach of both existing patch series?
>
   Plenty of details, thanks, Eric. I think Pavel's logic is better,
which increase the capability of it, and I don't think a more complex
logic should be there, since it is an internal function need clear
meaning.


>>
>>>
>>>> I'm also touching bdrv_snapshot_list where I'm adding an Error parameter
>>>     I looked it before, but it needs all call back in ./block support it,
>>> so is it really necessary?
>>
>> I think it is better if this function internally set appropriate error
>> message based on used disk image format (qcow2, sheepdog, rbd).
>
> I like the idea of find() setting errp on lookup failure.  Callers can
> ignore errors in situations where a failed lookup triggers a reasonable
> fallback, but in case the caller wants to report an error, the lower
> down that we generate an error message, the more likely the error
> message is to contain full context rather than being dumbed down by
> generating it higher on the call stack.
>
   I understand it helps, but now I need just a good
bdrv_snapshot_find() to use, so could this improvement work
be postponded later? I think neither my or Pavel's work
is depending on it.
   Pavel, to make us progress, I'd like a small serial change
bdrv_snapshot_find() first, then we can rebase on it, are u OK
with it?

block/qapi.c:
int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
                        const char *name, const char *id)
Wayne Xia - April 18, 2013, 7:20 a.m.
>> So, was I convincing enough in arguing that we want something different
>> from the approach of both existing patch series?
>>
>    Plenty of details, thanks, Eric. I think Pavel's logic is better,
> which increase the capability of it, and I don't think a more complex
> logic should be there, since it is an internal function need clear
> meaning.
>
>
>>>
>>
>    I understand it helps, but now I need just a good
> bdrv_snapshot_find() to use, so could this improvement work
> be postponded later? I think neither my or Pavel's work
> is depending on it.
>    Pavel, to make us progress, I'd like a small serial change
> bdrv_snapshot_find() first, then we can rebase on it, are u OK
> with it?
>
> block/qapi.c:
> int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>                         const char *name, const char *id)
>
>
>
   Actually my serial, two condition are "or" relationship,
but in Pavel's they are "and" relationship. From caller,
I think "and" is better, and taking two parameter in one function
make caller easier to use, in most case.
   I'd like a clean serial go for it, hope to know Pavel and
Markus's idea.
Pavel Hrdina - April 18, 2013, 10:22 a.m.
On 18.4.2013 06:31, Wenchao Xia wrote:
> 于 2013-4-18 2:14, Eric Blake 写道:
>> On 04/17/2013 04:51 AM, Pavel Hrdina wrote:
>>> On 17.4.2013 12:19, Wenchao Xia wrote:
>>>> 于 2013-4-17 15:52, Pavel Hrdina 写道:
>>>>> Hi Wenchao,
>>>>>
>>>>> unfortunately no. According to new design of savevm, loadvm and
>>>>> delvm I
>>>>> need also search for snapshots that have the specified name and id.
>>>>>
>>>>     It seems the logic in your function, is same with mine...
>>>
>>> It is not the same.
>>
>> Consider a qcow2 file with the following snapshots:
>>
>>       id     tag
>>       1      2
>>       2      1
>>       3      none
>>
>> Existing logic:
>>
>> only one string is passed, and only one loop is performed.  If it
>> matches id or name, end searching
>>
>> search for 1 - finds id 1 tag 2
>> search for 2 - finds id 1 tag 2
>> search for 3 - finds id 3 no tag
>> search for 4 - finds nothing
>>
>> no way to find id 2 tag 1
>>
>> The last point proves that the current algorithm is not ideal, and that
>> we are justified in changing it.  But there are several ways to change
>> it, and a consideration of whether we should preserve backwards
>> compatibility in HMP by making the search itself have backwards
>> compatibility, or by making the QMP search follow strict rules where HMP
>> can emulate some measure of back-compat by calling into QMP more than
>> once.
>>
>>>
>>> Your logic:
>> [Wenchao]
>>>      if id is set:
>>>          if there is snapshot with that id:
>>>              end searching
>>>      if name set (search also if id is set but nothing found):
>>>          if there is snapshot with that name:
>>>              end searching
>>>
>>> My logic:
>> [Pavel]
>>>      if name is set and id is set:
>>>          if there is snapshot with than name and with that id:
>>>              end searching
>>>      else if name is set (means that only name is set):
>>>          if there is snapshot with that name:
>>>              end searching
>>>      else if id is set (means that only id is set):
>>>          if there is snapshot with that id:
>>>              end searching
>>
>> Best is a side-by-side comparison; when comparing to existing, I showed
>> three different choices of expanding a single name into a two-argument
>> find call.
>>
>>   search for:            finds                compared to
>>    id     name     Wenchao       Pavel          existing
>>                                                name -> find(id, NULL)
>>    1      NULL    id 1 tag 2    id 1 tag 2    id 1 tag 2
>> * 2      NULL    id 2 tag 1    id 2 tag 1    id 1 tag 2
>>    3      NULL    id 3 no tag   id 3 no tag   id 3 no tag
>>    4      NULL    nothing       nothing       nothing
>>                                                name -> find(NULL, tag)
>> * NULL   1       id 2 tag 1    id 2 tag 1    id 1 tag 2
>>    NULL   2       id 1 tag 2    id 1 tag 2    id 1 tag 2
>> * NULL   3       nothing       nothing       id 3 no tag
>>    NULL   4       nothing       nothing       nothing
>>                                                not possible
>>    1      2       id 1 tag 2    id 1 tag 2    --
>>    2      1       id 2 tag 1    id 2 tag 1    --
>>                                                name -> find(id, tag)
>> * 1      1       id 1 tag 2    nothing       id 1 tag 2
>> * 2      2       id 2 tag 1    nothing       id 1 tag 2
>> * 3      3       id 3 no tag   nothing       id 3 no tag
>>    4      4       nothing       nothing       nothing
>>
>> The two proposed approaches both allow access to a lookup that the
>> original could not provide (namely, id 2 tag 1), so they are an
>> improvement on that front.  But the two approaches differ on behavior
>> when both id and name are specified (Wenchao's behaves the same as an
>> id-only lookup, regardless of whether the name matches; Pavel's requires
>> a double match).  The other thing to note is that the old code allowed a
>> single name to match an anonymous snapshot, but both proposals fail to
>> find a nameless snapshot without an id search.
>>
>> Can I put yet another proposed algorithm on the table?  First, written
>> with four loops over the source (although at most two are taken):
>>
>>      if name is set and id is set:
>>          if there is snapshot with than name and with that id (loop 1):
>>              end searching
>>      else if name is set (means that only name is set):
>>          if there is snapshot with that name (loop 2):
>>              end searching
>>          if there is snapshot with that id (loop 3):
>>              end searching
>>      else if id is set (means that only id is set):
>>          if there is snapshot with that id (loop 4):
>>              end searching
>>
>> Or, written another way, to implement the same results in only two coded
>> loops:
>>
>>      if name is set:
>>          if there is a snapshot with that name (loop 1):
>>              if id is set:
>>                  if id matches:
>>                      end searching successfully
>>                  else:
>>                      fail
>>              else:
>>                  end searching successfully
>>          else if id is not set:
>>              set id to name
>>      if there is a snapshot with id (loop 2):
>>          end searching successfully
>>
>> And the resulting comparison table, again with three possibilities of
>> how to convert one-argument lookup into two-argument:
>>
>>   search for:        finds      compared to
>>    id     name     mine          existing
>>                                  name -> find(id, NULL)
>>    1      NULL    id 1 tag 2    id 1 tag 2
>> * 2      NULL    id 2 tag 1    id 1 tag 2
>>    3      NULL    id 3 no tag   id 3 no tag
>>    4      NULL    nothing       nothing
>>                                  name -> find(NULL, tag)
>> * NULL   1       id 2 tag 1    id 1 tag 2
>>    NULL   2       id 1 tag 2    id 1 tag 2
>>    NULL   3       id 3 no tag   id 3 no tag
>>    NULL   4       nothing       nothing
>>                                  not possible
>>    1      2       id 1 tag 2    --
>>    2      1       id 2 tag 1    --
>>                                  name -> find(id, tag)
>> * 1      1       nothing       id 1 tag 2
>> * 2      2       nothing       id 1 tag 2
>> * 3      3       nothing       id 3 no tag
>>    4      4       nothing       nothing
>>
>>
>> With that adjusted table, I would favor converting any single name
>> lookup into find(NULL, tag).  Only the new QMP command that lets us do
>> an explicit id lookup can search for an id regardless of another name
>> matching the id; but we at least have the benefit of being able to
>> access ALL named snapshots (better than the original code unable to
>> access tag 2), as well as the ability to access unambiguous ids without
>> extra effort (ability to access id 3 without having to change the
>> command line).  It keeps the aspect of Pavel's code that specifying both
>> fields must match both fields, but otherwise favors names over ids
>> (generally good, since names are generally not numeric, except when we
>> are talking about corner cases).
>>
>> So, was I convincing enough in arguing that we want something different
>> from the approach of both existing patch series?
>>
>    Plenty of details, thanks, Eric. I think Pavel's logic is better,
> which increase the capability of it, and I don't think a more complex
> logic should be there, since it is an internal function need clear
> meaning.
>

I also thank you Eric for that detailed description. I must agree with 
Wenchao that internal function needs clear meaning. Why would I need to 
find snapshot by id using name parameter if I have the id parameter? I 
know that name is in almost all cases some string and not a number and 
that it could be for some cases easier just find snapshot by id using 
name parameter, but I think that we must be strict about this logic.

>
>>>
>>>>
>>>>> I'm also touching bdrv_snapshot_list where I'm adding an Error
>>>>> parameter
>>>>     I looked it before, but it needs all call back in ./block
>>>> support it,
>>>> so is it really necessary?
>>>
>>> I think it is better if this function internally set appropriate error
>>> message based on used disk image format (qcow2, sheepdog, rbd).
>>
>> I like the idea of find() setting errp on lookup failure.  Callers can
>> ignore errors in situations where a failed lookup triggers a reasonable
>> fallback, but in case the caller wants to report an error, the lower
>> down that we generate an error message, the more likely the error
>> message is to contain full context rather than being dumbed down by
>> generating it higher on the call stack.
>>
>    I understand it helps, but now I need just a good
> bdrv_snapshot_find() to use, so could this improvement work
> be postponded later? I think neither my or Pavel's work
> is depending on it.
>    Pavel, to make us progress, I'd like a small serial change
> bdrv_snapshot_find() first, then we can rebase on it, are u OK
> with it?
>
> block/qapi.c:
> int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>                         const char *name, const char *id)
>

I think that my whole series is almost done and it would be quickly 
accepted and applied upstream. I'll send today the v2 and we will see.

Pavel
Wayne Xia - April 19, 2013, 12:28 a.m.
于 2013-4-18 18:22, Pavel Hrdina 写道:
> On 18.4.2013 06:31, Wenchao Xia wrote:
>> 于 2013-4-18 2:14, Eric Blake 写道:
>>> On 04/17/2013 04:51 AM, Pavel Hrdina wrote:
>>>> On 17.4.2013 12:19, Wenchao Xia wrote:
>>>>> 于 2013-4-17 15:52, Pavel Hrdina 写道:
>>>>>> Hi Wenchao,
>>>>>>
>>>>>> unfortunately no. According to new design of savevm, loadvm and
>>>>>> delvm I
>>>>>> need also search for snapshots that have the specified name and id.
>>>>>>
>>>>>     It seems the logic in your function, is same with mine...
>>>>
>>>> It is not the same.
>>>
>>> Consider a qcow2 file with the following snapshots:
>>>
>>>       id     tag
>>>       1      2
>>>       2      1
>>>       3      none
>>>
>>> Existing logic:
>>>
>>> only one string is passed, and only one loop is performed.  If it
>>> matches id or name, end searching
>>>
>>> search for 1 - finds id 1 tag 2
>>> search for 2 - finds id 1 tag 2
>>> search for 3 - finds id 3 no tag
>>> search for 4 - finds nothing
>>>
>>> no way to find id 2 tag 1
>>>
>>> The last point proves that the current algorithm is not ideal, and that
>>> we are justified in changing it.  But there are several ways to change
>>> it, and a consideration of whether we should preserve backwards
>>> compatibility in HMP by making the search itself have backwards
>>> compatibility, or by making the QMP search follow strict rules where HMP
>>> can emulate some measure of back-compat by calling into QMP more than
>>> once.
>>>
>>>>
>>>> Your logic:
>>> [Wenchao]
>>>>      if id is set:
>>>>          if there is snapshot with that id:
>>>>              end searching
>>>>      if name set (search also if id is set but nothing found):
>>>>          if there is snapshot with that name:
>>>>              end searching
>>>>
>>>> My logic:
>>> [Pavel]
>>>>      if name is set and id is set:
>>>>          if there is snapshot with than name and with that id:
>>>>              end searching
>>>>      else if name is set (means that only name is set):
>>>>          if there is snapshot with that name:
>>>>              end searching
>>>>      else if id is set (means that only id is set):
>>>>          if there is snapshot with that id:
>>>>              end searching
>>>
>>> Best is a side-by-side comparison; when comparing to existing, I showed
>>> three different choices of expanding a single name into a two-argument
>>> find call.
>>>
>>>   search for:            finds                compared to
>>>    id     name     Wenchao       Pavel          existing
>>>                                                name -> find(id, NULL)
>>>    1      NULL    id 1 tag 2    id 1 tag 2    id 1 tag 2
>>> * 2      NULL    id 2 tag 1    id 2 tag 1    id 1 tag 2
>>>    3      NULL    id 3 no tag   id 3 no tag   id 3 no tag
>>>    4      NULL    nothing       nothing       nothing
>>>                                                name -> find(NULL, tag)
>>> * NULL   1       id 2 tag 1    id 2 tag 1    id 1 tag 2
>>>    NULL   2       id 1 tag 2    id 1 tag 2    id 1 tag 2
>>> * NULL   3       nothing       nothing       id 3 no tag
>>>    NULL   4       nothing       nothing       nothing
>>>                                                not possible
>>>    1      2       id 1 tag 2    id 1 tag 2    --
>>>    2      1       id 2 tag 1    id 2 tag 1    --
>>>                                                name -> find(id, tag)
>>> * 1      1       id 1 tag 2    nothing       id 1 tag 2
>>> * 2      2       id 2 tag 1    nothing       id 1 tag 2
>>> * 3      3       id 3 no tag   nothing       id 3 no tag
>>>    4      4       nothing       nothing       nothing
>>>
>>> The two proposed approaches both allow access to a lookup that the
>>> original could not provide (namely, id 2 tag 1), so they are an
>>> improvement on that front.  But the two approaches differ on behavior
>>> when both id and name are specified (Wenchao's behaves the same as an
>>> id-only lookup, regardless of whether the name matches; Pavel's requires
>>> a double match).  The other thing to note is that the old code allowed a
>>> single name to match an anonymous snapshot, but both proposals fail to
>>> find a nameless snapshot without an id search.
>>>
>>> Can I put yet another proposed algorithm on the table?  First, written
>>> with four loops over the source (although at most two are taken):
>>>
>>>      if name is set and id is set:
>>>          if there is snapshot with than name and with that id (loop 1):
>>>              end searching
>>>      else if name is set (means that only name is set):
>>>          if there is snapshot with that name (loop 2):
>>>              end searching
>>>          if there is snapshot with that id (loop 3):
>>>              end searching
>>>      else if id is set (means that only id is set):
>>>          if there is snapshot with that id (loop 4):
>>>              end searching
>>>
>>> Or, written another way, to implement the same results in only two coded
>>> loops:
>>>
>>>      if name is set:
>>>          if there is a snapshot with that name (loop 1):
>>>              if id is set:
>>>                  if id matches:
>>>                      end searching successfully
>>>                  else:
>>>                      fail
>>>              else:
>>>                  end searching successfully
>>>          else if id is not set:
>>>              set id to name
>>>      if there is a snapshot with id (loop 2):
>>>          end searching successfully
>>>
>>> And the resulting comparison table, again with three possibilities of
>>> how to convert one-argument lookup into two-argument:
>>>
>>>   search for:        finds      compared to
>>>    id     name     mine          existing
>>>                                  name -> find(id, NULL)
>>>    1      NULL    id 1 tag 2    id 1 tag 2
>>> * 2      NULL    id 2 tag 1    id 1 tag 2
>>>    3      NULL    id 3 no tag   id 3 no tag
>>>    4      NULL    nothing       nothing
>>>                                  name -> find(NULL, tag)
>>> * NULL   1       id 2 tag 1    id 1 tag 2
>>>    NULL   2       id 1 tag 2    id 1 tag 2
>>>    NULL   3       id 3 no tag   id 3 no tag
>>>    NULL   4       nothing       nothing
>>>                                  not possible
>>>    1      2       id 1 tag 2    --
>>>    2      1       id 2 tag 1    --
>>>                                  name -> find(id, tag)
>>> * 1      1       nothing       id 1 tag 2
>>> * 2      2       nothing       id 1 tag 2
>>> * 3      3       nothing       id 3 no tag
>>>    4      4       nothing       nothing
>>>
>>>
>>> With that adjusted table, I would favor converting any single name
>>> lookup into find(NULL, tag).  Only the new QMP command that lets us do
>>> an explicit id lookup can search for an id regardless of another name
>>> matching the id; but we at least have the benefit of being able to
>>> access ALL named snapshots (better than the original code unable to
>>> access tag 2), as well as the ability to access unambiguous ids without
>>> extra effort (ability to access id 3 without having to change the
>>> command line).  It keeps the aspect of Pavel's code that specifying both
>>> fields must match both fields, but otherwise favors names over ids
>>> (generally good, since names are generally not numeric, except when we
>>> are talking about corner cases).
>>>
>>> So, was I convincing enough in arguing that we want something different
>>> from the approach of both existing patch series?
>>>
>>    Plenty of details, thanks, Eric. I think Pavel's logic is better,
>> which increase the capability of it, and I don't think a more complex
>> logic should be there, since it is an internal function need clear
>> meaning.
>>
>
> I also thank you Eric for that detailed description. I must agree with
> Wenchao that internal function needs clear meaning. Why would I need to
> find snapshot by id using name parameter if I have the id parameter? I
> know that name is in almost all cases some string and not a number and
> that it could be for some cases easier just find snapshot by id using
> name parameter, but I think that we must be strict about this logic.
>
>>
>>>>
>>>>>
>>>>>> I'm also touching bdrv_snapshot_list where I'm adding an Error
>>>>>> parameter
>>>>>     I looked it before, but it needs all call back in ./block
>>>>> support it,
>>>>> so is it really necessary?
>>>>
>>>> I think it is better if this function internally set appropriate error
>>>> message based on used disk image format (qcow2, sheepdog, rbd).
>>>
>>> I like the idea of find() setting errp on lookup failure.  Callers can
>>> ignore errors in situations where a failed lookup triggers a reasonable
>>> fallback, but in case the caller wants to report an error, the lower
>>> down that we generate an error message, the more likely the error
>>> message is to contain full context rather than being dumbed down by
>>> generating it higher on the call stack.
>>>
>>    I understand it helps, but now I need just a good
>> bdrv_snapshot_find() to use, so could this improvement work
>> be postponded later? I think neither my or Pavel's work
>> is depending on it.
>>    Pavel, to make us progress, I'd like a small serial change
>> bdrv_snapshot_find() first, then we can rebase on it, are u OK
>> with it?
>>
>> block/qapi.c:
>> int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>                         const char *name, const char *id)
>>
>
> I think that my whole series is almost done and it would be quickly
> accepted and applied upstream. I'll send today the v2 and we will see.
>
> Pavel
>
   OK, I'll review v2 too to make it faster.
Wayne Xia - April 24, 2013, 3:51 a.m.
>>
>> I think that my whole series is almost done and it would be quickly
>> accepted and applied upstream. I'll send today the v2 and we will see.
>>
>> Pavel
>>
>    OK, I'll review v2 too to make it faster.
>
Hi, Pavel
   Sorry for query your progress, I hope to catch up qemu 1.5, so
wonder whether V2 is ready.
   If you agree, I will use your logic in my bdrv_snapshot_find() to
progress. But still, if you can upstream your version quickly, or form
a clean patch for this function, I am happy to rebase on it.:>
Pavel Hrdina - April 24, 2013, 9:37 a.m.
On 24.4.2013 05:51, Wenchao Xia wrote:
>
>>>
>>> I think that my whole series is almost done and it would be quickly
>>> accepted and applied upstream. I'll send today the v2 and we will see.
>>>
>>> Pavel
>>>
>>    OK, I'll review v2 too to make it faster.
>>
> Hi, Pavel
>    Sorry for query your progress, I hope to catch up qemu 1.5, so
> wonder whether V2 is ready.
>    If you agree, I will use your logic in my bdrv_snapshot_find() to
> progress. But still, if you can upstream your version quickly, or form
> a clean patch for this function, I am happy to rebase on it.:>
>

Hi Wenchao,

np, I have the v2 almost ready and today I'll send that v2 into upstream 
for review.

Patch

diff --git a/savevm.c b/savevm.c
index 66753da..bc829a5 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2195,7 +2195,7 @@  out:
 }
 
 static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
-                              const char *name, const char *id, bool old_match)
+                              const char *name, const char *id)
 {
     QEMUSnapshotInfo *sn_tab, *sn;
     int nb_sns, i, found = 0;
@@ -2218,20 +2218,10 @@  static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
                 break;
             }
         } else if (name) {
-            /* for compatibility for old bdrv_snapshot_find call
-             * will be removed */
-            if (old_match) {
-                if (!strcmp(sn->id_str, id) || !strcmp(sn->name, name)) {
-                    *sn_info = *sn;
-                    found = 1;
-                    break;
-                }
-            } else {
-                if (!strcmp(sn->name, name)) {
-                    *sn_info = *sn;
-                    found = 1;
-                    break;
-                }
+            if (!strcmp(sn->name, name)) {
+                *sn_info = *sn;
+                found = 1;
+                break;
             }
         } else if (id) {
             if (!strcmp(sn->id_str, id)) {
@@ -2290,7 +2280,7 @@  SnapshotInfo *qmp_vm_snapshot_save(const char *name, Error **errp)
     sn->date_nsec = tv.tv_usec * 1000;
     sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
 
-    if (bdrv_snapshot_find(bs, old_sn, name, NULL, false)) {
+    if (bdrv_snapshot_find(bs, old_sn, name, NULL)) {
         error_setg(errp, "snapshot '%s' exists", name);
         goto the_end;
     } else {
@@ -2388,7 +2378,7 @@  SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
     }
 
     /* Don't even try to load empty VM states */
-    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, false)) {
+    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id)) {
         return NULL;
     }
 
@@ -2413,7 +2403,7 @@  SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
             return NULL;
         }
 
-        if (!bdrv_snapshot_find(bs, &sn, name, id, false)) {
+        if (!bdrv_snapshot_find(bs, &sn, name, id)) {
             return NULL;
         }
     }
@@ -2484,7 +2474,7 @@  SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
         return NULL;
     }
 
-    if (!bdrv_snapshot_find(bs, &sn, name, id, false)) {
+    if (!bdrv_snapshot_find(bs, &sn, name, id)) {
         /* no need to set an error if snapshot doesn't exist */
         return NULL;
     }
@@ -2501,7 +2491,7 @@  SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
     bs = NULL;
     while ((bs = bdrv_next(bs))) {
         if (bdrv_can_snapshot(bs)
-                && bdrv_snapshot_find(bs, &sn, name, id, false)) {
+                && bdrv_snapshot_find(bs, &sn, name, id)) {
             bdrv_snapshot_delete(bs, sn.name, errp);
             if (error_is_set(errp)) {
                 return NULL;
@@ -2549,8 +2539,7 @@  void do_info_snapshots(Monitor *mon, const QDict *qdict)
 
         while ((bs1 = bdrv_next(bs1))) {
             if (bdrv_can_snapshot(bs1) && bs1 != bs) {
-                if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str,
-                                        true)) {
+                if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str)) {
                     available = 0;
                     break;
                 }