[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter |
Date: |
Thu, 25 Apr 2013 06:16:03 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130402 Thunderbird/17.0.5 |
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
[Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm, Pavel Hrdina, 2013/04/24
[Qemu-devel] [PATCH v2 10/12] savevm: update error reporting of qemu_savevm_state() and related functions, Pavel Hrdina, 2013/04/24
[Qemu-devel] [PATCH v2 08/12] qapi: Convert loadvm, Pavel Hrdina, 2013/04/24
[Qemu-devel] [PATCH v2 09/12] block: update error reporting for bdrv_snapshot_create() and related functions, Pavel Hrdina, 2013/04/24
[Qemu-devel] [PATCH v2 05/12] block: update error reporting for bdrv_snapshot_goto() and related functions, Pavel Hrdina, 2013/04/24