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.