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

[Pavel Hrdina <phrdina@redhat.com>]

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