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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature