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

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 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.

-- 
Best Regards

Wenchao Xia