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

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 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.

-- 
Best Regards

Wenchao Xia