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

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

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



-- 
Best Regards

Wenchao Xia