[libvirt] [PATCHv2 02/14] event: make deregister return value match docs
jferlan at redhat.com
Tue Jan 7 13:09:05 UTC 2014
On 01/06/2014 05:27 PM, Eric Blake wrote:
> Ever since their introduction (commit 1509b80 in v0.5.0 for
> virConnectDomainEventRegister, commit 4445723 in v0.8.0 for
> virConnectDomainEventDeregisterAny), the event deregistration
> functions have been documented as returning 0 on success;
> likewise for older registration (only the newer RegisterAny
> must return a non-zero callbackID). And now that we are
> adding virConnectNetworkEventDeregisterAny for v1.2.1, it
> should have the same semantics.
> Fortunately, all of the stateful drivers have been obeying
> the docs and returning 0, thanks to the way the remote_driver
> tracks things (in fact, the RPC wire protocol is unable to
> send a return value for DomainEventRegisterAny, at least not
> without adding a new RPC number). Well, except for vbox,
> which was always failing, due to failure to set the return
> value to anything besides its initial -1 value.
> But for local drivers, such as test:///default, we've been
> returning non-zero numbers; worse, the non-zero numbers have
> differed over time. For example, in Fedora 12 (libvirt 0.8.2),
> calling Register twice would return 0 and 1 [the callbackID
> generated under the hood]; while in Fedora 20 (libvirt 1.1.3),
> it returns 1 and 2 [the number of callbacks registered for
> that event type]. Since we have changed the behavior over
> time, and since it differs by local vs. remote, we can safely
> argue that no one could have been reasonably relying on any
> particular behavior, so we might as well obey the docs, as well
> as prepare callers that might deal with older clients to not be
> surprised if the docs are not strictly followed.
> For consistency, this patch fixes the code for all drivers,
> even though it only makes an impact for vbox and for local
> drivers. By fixing all drivers, future copy and paste from
> a remote driver to a local driver is less likely to
> reintroduce the bug.
> * src/libvirt.c (virConnectDomainEventRegister)
> (virConnectDomainEventDeregisterAny): Clarify docs.
> * src/libxl/libxl_driver.c (libxlConnectDomainEventRegister)
> (libxlConnectDomainEventDeregisterAny): Match documentation.
> * src/lxc/lxc_driver.c (lxcConnectDomainEventRegister)
> (lxcConnectDomainEventDeregisterAny): Likewise.
> * src/test/test_driver.c (testConnectDomainEventRegister)
> (testConnectNetworkEventDeregisterAny): Likewise.
> * src/uml/uml_driver.c (umlConnectDomainEventRegister)
> (umlConnectDomainEventDeregisterAny): Likewise.
> * src/vbox/vbox_tmpl.c (vboxConnectDomainEventRegister)
> (vboxConnectDomainEventDeregisterAny): Likewise.
> * src/xen/xen_driver.c (xenUnifiedConnectDomainEventRegister)
> (xenUnifiedConnectDomainEventDeregisterAny): Likewise.
> * src/network/bridge_driver.c
> (networkConnectNetworkEventDeregisterAny): Likewise.
> Signed-off-by: Eric Blake <eblake at redhat.com>
> src/libvirt.c | 17 +++++++++++------
> src/libxl/libxl_driver.c | 35 ++++++++++++++++++-----------------
> src/lxc/lxc_driver.c | 30 +++++++++++++++---------------
> src/network/bridge_driver.c | 11 +++++++----
> src/test/test_driver.c | 38 +++++++++++++++++++++-----------------
> src/uml/uml_driver.c | 29 ++++++++++++++++-------------
> src/vbox/vbox_tmpl.c | 32 ++++++++++++++++++++++----------
> src/xen/xen_driver.c | 25 ++++++++++++++-----------
> 8 files changed, 124 insertions(+), 93 deletions(-)
Since they're a match set... I'll ACK 1 & 2 together.
I was going to comment about vbox_tmpl.c, but some things are just
better left alone :-)
More information about the libvir-list