Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2331 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]