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]

于 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