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

[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?

> 
>>
>>> 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   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature