于 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
