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) > (virConnectDomainEventDeregister) > (virConnectDomainEventDeregisterAny): Clarify docs. > * src/libxl/libxl_driver.c (libxlConnectDomainEventRegister) > (libxlConnectDomainEventDeregister) > (libxlConnectDomainEventDeregisterAny): Match documentation. > * src/lxc/lxc_driver.c (lxcConnectDomainEventRegister) > (lxcConnectDomainEventDeregister) > (lxcConnectDomainEventDeregisterAny): Likewise. > * src/test/test_driver.c (testConnectDomainEventRegister) > (testConnectDomainEventDeregister) > (testConnectDomainEventDeregisterAny) > (testConnectNetworkEventDeregisterAny): Likewise. > * src/uml/uml_driver.c (umlConnectDomainEventRegister) > (umlConnectDomainEventDeregister) > (umlConnectDomainEventDeregisterAny): Likewise. > * src/vbox/vbox_tmpl.c (vboxConnectDomainEventRegister) > (vboxConnectDomainEventDeregister) > (vboxConnectDomainEventDeregisterAny): Likewise. > * src/xen/xen_driver.c (xenUnifiedConnectDomainEventRegister) > (xenUnifiedConnectDomainEventDeregister) > (xenUnifiedConnectDomainEventDeregisterAny): Likewise. > * src/network/bridge_driver.c > (networkConnectNetworkEventDeregisterAny): Likewise. > > Signed-off-by: Eric Blake <eblake@xxxxxxxxxx> > --- > 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 :-) John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list