Re: [Qemu-devel] [PATCH 11/11] savevm: remove backward compatibility from bdrv_snapshot_find()

[Pavel Hrdina]

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