Comments

Finding snapshot by a name which could also be an id isn't best way
how to do it. There will be rewrite of savevm, loadvm and delvm to
improve the behavior of these commands. The savevm and loadvm will
have their own patch series.
Now bdrv_snapshot_find takes more parameters. The name parameter will
be matched only against the name of the snapshot and the same applies
to id parameter.
There is one exception. If you set the last parameter, the name parameter
will be matched against the name or the id of a snapshot. This exception
is only for backward compatibility for other commands and it will be
dropped after all commands will be rewritten.
We only need to know if that snapshot exists or not. We don't care
about any error message. If snapshot exists it returns TRUE otherwise
it returns FALSE.
There is also new Error parameter which will containt error messeage if
something goes wrong.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
savevm.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++------------------
1 file changed, 67 insertions(+), 26 deletions(-)

On 04/25/2013 12:31 AM, Wenchao Xia wrote:
> >> +>> + if (!found) {>> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id);> suggest not to set error, since it is a normal case.
The way I understand it, failure to find a snapshot might need to be
treated as an error - it's up to the caller's needs. Also, there pretty
much is only one failure mode - the requested snapshot was not found -
even if there are multiple ways that we can fail to find a requested
snapshot, so I'm fine with treating all 'false' returns as an error path.
Thus, a caller that wants to probe for a snapshot existence but not set
an error calls:
bdrv_snapshot_find(bs, snapshot, name, id, NULL, false);
while a caller that wants to report a missing snapshot as an error calls:
bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false);
and then propagates local_err on upwards.
Or are you worried about a possible third case, where a caller cares
about failure during bdrv_snapshot_list(), differently than failure to
find a snapshot? What callers have that semantics? If that is a real
concern, then maybe returning a bool is the wrong approach, and we
should instead return an int. A return < 0 is a fatal error
(bdrv_snapshot_list failed to even look up snapshots); a return of 0
means our lookup attempt hit no fatal errors but the snapshot was not
found, and a return of 1 means the snapshot was found. Then there would
be three calling styles:
Probe for existence, with no error reporting:
if (bdrv_snapshot_find(bs, snapshot, name, id, NULL, false) > 0) {
// exists
}
Probe for existence but with error reporting on fatal errors:
exist = bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false);
if (exist < 0) {
// propagate local_err
} else if (exist) {
// exists
}
Probe for snapshot, with error reporting even for failed lookup:
if (bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false) <= 0) {
// propagate local_err
}
But I don't know what the existing callers need, to make a decision on
whether a signature change is warranted. Again, more reason to defer
this series to 1.6.

于 2013-4-25 20:16, Eric Blake 写道:
> On 04/25/2013 12:31 AM, Wenchao Xia wrote:>>>>> +>>> + if (!found) {>>> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id);>> suggest not to set error, since it is a normal case.>> The way I understand it, failure to find a snapshot might need to be> treated as an error - it's up to the caller's needs. Also, there pretty> much is only one failure mode - the requested snapshot was not found -> even if there are multiple ways that we can fail to find a requested> snapshot, so I'm fine with treating all 'false' returns as an error path.>> Thus, a caller that wants to probe for a snapshot existence but not set> an error calls:> bdrv_snapshot_find(bs, snapshot, name, id, NULL, false);>> while a caller that wants to report a missing snapshot as an error calls:> bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false);> and then propagates local_err on upwards.>>> Or are you worried about a possible third case, where a caller cares> about failure during bdrv_snapshot_list(), differently than failure to> find a snapshot? What callers have that semantics? If that is a real> concern, then maybe returning a bool is the wrong approach, and we> should instead return an int. A return < 0 is a fatal error> (bdrv_snapshot_list failed to even look up snapshots); a return of 0> means our lookup attempt hit no fatal errors but the snapshot was not> found, and a return of 1 means the snapshot was found. Then there would> be three calling styles:>> Probe for existence, with no error reporting:> if (bdrv_snapshot_find(bs, snapshot, name, id, NULL, false) > 0) {> // exists> }> Probe for existence but with error reporting on fatal errors:> exist = bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false);> if (exist < 0) {> // propagate local_err> } else if (exist) {> // exists> }> Probe for snapshot, with error reporting even for failed lookup:> if (bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false) <= 0) {> // propagate local_err> }>> But I don't know what the existing callers need, to make a decision on> whether a signature change is warranted. Again, more reason to defer> this series to 1.6.>
Personally I prefer internal layer have clean meaning, setting error
only for exception. But I am not strongly against it, if caller can
make easier use of it, a document for this function is also OK.