[libvirt] [PATCH 2/2] event: make deregister return value match docs
Osier Yang
jyang at redhat.com
Mon Jan 6 11:16:23 UTC 2014
On 04/01/14 03:31, 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). 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 local drivers; that
> way, 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 at redhat.com>
> ---
> src/libvirt.c | 17 +++++++++++------
> src/libxl/libxl_driver.c | 35 ++++++++++++++++++-----------------
> src/lxc/lxc_driver.c | 32 ++++++++++++++++----------------
> 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 | 27 +++++++++++++++------------
> 8 files changed, 126 insertions(+), 95 deletions(-)
>
> diff --git a/src/libvirt.c b/src/libvirt.c
> index a0a26e5..f527dcc 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -16363,7 +16363,9 @@ error:
> * The reference can be released once the object is no longer required
> * by calling virDomainFree.
> *
> - * Returns 0 on success, -1 on failure.
> + * Returns 0 on success, -1 on failure. Older versions of some hypervisors
> + * sometimes returned a positive number on success, but without any reliable
> + * semantics on what that number represents.
> */
> int
> virConnectDomainEventRegister(virConnectPtr conn,
> @@ -16401,14 +16403,16 @@ error:
> * @conn: pointer to the connection
> * @cb: callback to the function handling domain events
> *
> - * Removes a callback previously registered with the virConnectDomainEventRegister
> - * function.
> + * Removes a callback previously registered with the
> + * virConnectDomainEventRegister() function.
> *
> * Use of this method is no longer recommended. Instead applications
> * should try virConnectDomainEventDeregisterAny() which has a more flexible
> * API contract
> *
> - * Returns 0 on success, -1 on failure
> + * Returns 0 on success, -1 on failure. Older versions of some hypervisors
> + * sometimes returned a positive number on success, but without any reliable
> + * semantics on what that number represents.
> */
> int
> virConnectDomainEventDeregister(virConnectPtr conn,
> @@ -19443,8 +19447,9 @@ error:
> * Removes an event callback. The callbackID parameter should be the
> * value obtained from a previous virConnectDomainEventRegisterAny() method.
> *
> - * Returns 0 on success, -1 on failure
> - */
> + * Returns 0 on success, -1 on failure. Older versions of some hypervisors
> + * sometimes returned a positive number on success, but without any reliable
> + * semantics on what that number represents. */
> int
> virConnectDomainEventDeregisterAny(virConnectPtr conn,
> int callbackID)
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index b24c195..61e3516 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -1,7 +1,7 @@
> /*
> * libxl_driver.c: core driver methods for managing libxenlight domains
> *
> - * Copyright (C) 2006-2013 Red Hat, Inc.
> + * Copyright (C) 2006-2014 Red Hat, Inc.
> * Copyright (C) 2011-2013 SUSE LINUX Products GmbH, Nuernberg, Germany.
> * Copyright (C) 2011 Univention GmbH.
> *
> @@ -3643,20 +3643,21 @@ cleanup:
>
> static int
> libxlConnectDomainEventRegister(virConnectPtr conn,
> - virConnectDomainEventCallback callback, void *opaque,
> + virConnectDomainEventCallback callback,
> + void *opaque,
> virFreeCallback freecb)
> {
> libxlDriverPrivatePtr driver = conn->privateData;
> - int ret;
>
> if (virConnectDomainEventRegisterEnsureACL(conn) < 0)
> return -1;
>
> - ret = virDomainEventStateRegister(conn,
> - driver->domainEventState,
> - callback, opaque, freecb);
> + if (virDomainEventStateRegister(conn,
> + driver->domainEventState,
> + callback, opaque, freecb) < 0)
> + return -1;
>
> - return ret;
> + return 0;
> }
>
>
> @@ -3665,16 +3666,16 @@ libxlConnectDomainEventDeregister(virConnectPtr conn,
> virConnectDomainEventCallback callback)
> {
> libxlDriverPrivatePtr driver = conn->privateData;
> - int ret;
>
> if (virConnectDomainEventDeregisterEnsureACL(conn) < 0)
> return -1;
>
> - ret = virDomainEventStateDeregister(conn,
> - driver->domainEventState,
> - callback);
> + if (virDomainEventStateDeregister(conn,
> + driver->domainEventState,
> + callback) < 0)
> + return -1;
>
> - return ret;
> + return 0;
> }
>
> static int
> @@ -4270,16 +4271,16 @@ static int
> libxlConnectDomainEventDeregisterAny(virConnectPtr conn, int callbackID)
> {
> libxlDriverPrivatePtr driver = conn->privateData;
> - int ret;
>
> if (virConnectDomainEventDeregisterAnyEnsureACL(conn) < 0)
> return -1;
>
> - ret = virObjectEventStateDeregisterID(conn,
> - driver->domainEventState,
> - callbackID);
> + if (virObjectEventStateDeregisterID(conn,
> + driver->domainEventState,
> + callbackID) < 0)
> + return -1;
>
> - return ret;
> + return 0;
> }
>
>
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 77b25d4..e0e3fd4 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (C) 2010-2013 Red Hat, Inc.
> + * Copyright (C) 2010-2014 Red Hat, Inc.
> * Copyright IBM Corp. 2008
> *
> * lxc_driver.c: linux container driver functions
> @@ -1287,16 +1287,16 @@ lxcConnectDomainEventRegister(virConnectPtr conn,
> virFreeCallback freecb)
> {
> virLXCDriverPtr driver = conn->privateData;
> - int ret;
>
> if (virConnectDomainEventRegisterEnsureACL(conn) < 0)
> return -1;
>
> - ret = virDomainEventStateRegister(conn,
> - driver->domainEventState,
> - callback, opaque, freecb);
> + if (virDomainEventStateRegister(conn,
> + driver->domainEventState,
> + callback, opaque, freecb) < 0)
> + return -1;
>
> - return ret;
> + return 0;
> }
>
>
> @@ -1305,16 +1305,16 @@ lxcConnectDomainEventDeregister(virConnectPtr conn,
> virConnectDomainEventCallback callback)
> {
> virLXCDriverPtr driver = conn->privateData;
> - int ret;
>
> if (virConnectDomainEventDeregisterEnsureACL(conn) < 0)
> return -1;
>
> - ret = virDomainEventStateDeregister(conn,
> - driver->domainEventState,
> - callback);
> + if (virDomainEventStateDeregister(conn,
> + driver->domainEventState,
> + callback) < 0)
> + return -1;
>
> - return ret;
> + return 0;
> }
>
>
> @@ -1347,16 +1347,16 @@ lxcConnectDomainEventDeregisterAny(virConnectPtr conn,
> int callbackID)
> {
> virLXCDriverPtr driver = conn->privateData;
> - int ret;
>
> if (virConnectDomainEventDeregisterAnyEnsureACL(conn) < 0)
> return -1;
>
> - ret = virObjectEventStateDeregisterID(conn,
> - driver->domainEventState,
> - callbackID);
> + if (virObjectEventStateDeregisterID(conn,
> + driver->domainEventState,
> + callbackID) < 0)
> + return -1;
>
> - return ret;
> + return 0;
> }
>
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 3e10758..9dc1f7e 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1,7 +1,7 @@
> /*
> * bridge_driver.c: core driver methods for managing network
> *
> - * Copyright (C) 2006-2013 Red Hat, Inc.
> + * Copyright (C) 2006-2014 Red Hat, Inc.
> * Copyright (C) 2006 Daniel P. Berrange
> *
> * This library is free software; you can redistribute it and/or
> @@ -2329,9 +2329,12 @@ networkConnectNetworkEventDeregisterAny(virConnectPtr conn,
> if (virConnectNetworkEventDeregisterAnyEnsureACL(conn) < 0)
> goto cleanup;
>
> - ret = virObjectEventStateDeregisterID(conn,
> - driver->networkEventState,
> - callbackID);
> + if (virObjectEventStateDeregisterID(conn,
> + driver->networkEventState,
> + callbackID) < 0)
> + goto cleanup;
> +
> + ret = 0;
>
> cleanup:
> return ret;
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index a48404a..d0f3fa8 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -1,7 +1,7 @@
> /*
> * test.c: A "mock" hypervisor for use by application unit tests
> *
> - * Copyright (C) 2006-2013 Red Hat, Inc.
> + * Copyright (C) 2006-2014 Red Hat, Inc.
> * Copyright (C) 2006 Daniel P. Berrange
> *
> * This library is free software; you can redistribute it and/or
> @@ -5993,12 +5993,13 @@ testConnectDomainEventRegister(virConnectPtr conn,
> virFreeCallback freecb)
> {
> testConnPtr driver = conn->privateData;
> - int ret;
> + int ret = 0;
>
> testDriverLock(driver);
> - ret = virDomainEventStateRegister(conn,
> - driver->domainEventState,
> - callback, opaque, freecb);
> + if (virDomainEventStateRegister(conn,
> + driver->domainEventState,
> + callback, opaque, freecb) < 0)
> + ret = -1;
> testDriverUnlock(driver);
>
> return ret;
> @@ -6010,12 +6011,13 @@ testConnectDomainEventDeregister(virConnectPtr conn,
> virConnectDomainEventCallback callback)
> {
> testConnPtr driver = conn->privateData;
> - int ret;
> + int ret = 0;
>
> testDriverLock(driver);
> - ret = virDomainEventStateDeregister(conn,
> - driver->domainEventState,
> - callback);
> + if (virDomainEventStateDeregister(conn,
> + driver->domainEventState,
> + callback) < 0)
> + ret = -1;
> testDriverUnlock(driver);
>
> return ret;
> @@ -6049,12 +6051,13 @@ testConnectDomainEventDeregisterAny(virConnectPtr conn,
> int callbackID)
> {
> testConnPtr driver = conn->privateData;
> - int ret;
> + int ret = 0;
>
> testDriverLock(driver);
> - ret = virObjectEventStateDeregisterID(conn,
> - driver->domainEventState,
> - callbackID);
> + if (virObjectEventStateDeregisterID(conn,
> + driver->domainEventState,
> + callbackID) < 0)
> + ret = -1;
> testDriverUnlock(driver);
>
> return ret;
> @@ -6089,12 +6092,13 @@ testConnectNetworkEventDeregisterAny(virConnectPtr conn,
> int callbackID)
> {
> testConnPtr driver = conn->privateData;
> - int ret;
> + int ret = 0;
>
> testDriverLock(driver);
> - ret = virObjectEventStateDeregisterID(conn,
> - driver->domainEventState,
> - callbackID);
> + if (virObjectEventStateDeregisterID(conn,
> + driver->domainEventState,
> + callbackID) < 0)
> + ret = -1;
> testDriverUnlock(driver);
>
> return ret;
> diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
> index 1784eb5..ad29ebf 100644
> --- a/src/uml/uml_driver.c
> +++ b/src/uml/uml_driver.c
> @@ -1,7 +1,7 @@
> /*
> * uml_driver.c: core driver methods for managing UML guests
> *
> - * Copyright (C) 2006-2013 Red Hat, Inc.
> + * Copyright (C) 2006-2014 Red Hat, Inc.
> * Copyright (C) 2006-2008 Daniel P. Berrange
> *
> * This library is free software; you can redistribute it and/or
> @@ -2610,15 +2610,16 @@ umlConnectDomainEventRegister(virConnectPtr conn,
> virFreeCallback freecb)
> {
> struct uml_driver *driver = conn->privateData;
> - int ret;
> + int ret = 0;
>
> if (virConnectDomainEventRegisterEnsureACL(conn) < 0)
> return -1;
>
> umlDriverLock(driver);
> - ret = virDomainEventStateRegister(conn,
> - driver->domainEventState,
> - callback, opaque, freecb);
> + if (virDomainEventStateRegister(conn,
> + driver->domainEventState,
> + callback, opaque, freecb) < 0)
> + ret = -1;
> umlDriverUnlock(driver);
>
> return ret;
> @@ -2629,15 +2630,16 @@ umlConnectDomainEventDeregister(virConnectPtr conn,
> virConnectDomainEventCallback callback)
> {
> struct uml_driver *driver = conn->privateData;
> - int ret;
> + int ret = 0;
>
> if (virConnectDomainEventDeregisterEnsureACL(conn) < 0)
> return -1;
>
> umlDriverLock(driver);
> - ret = virDomainEventStateDeregister(conn,
> - driver->domainEventState,
> - callback);
> + if (virDomainEventStateDeregister(conn,
> + driver->domainEventState,
> + callback) < 0)
> + ret = -1;
> umlDriverUnlock(driver);
>
> return ret;
> @@ -2674,15 +2676,16 @@ umlConnectDomainEventDeregisterAny(virConnectPtr conn,
> int callbackID)
> {
> struct uml_driver *driver = conn->privateData;
> - int ret;
> + int ret = 0;
>
> if (virConnectDomainEventDeregisterAnyEnsureACL(conn) < 0)
> return -1;
>
> umlDriverLock(driver);
> - ret = virObjectEventStateDeregisterID(conn,
> - driver->domainEventState,
> - callbackID);
> + if (virObjectEventStateDeregisterID(conn,
> + driver->domainEventState,
> + callbackID) < 0)
> + ret = -1;
> umlDriverUnlock(driver);
>
> return ret;
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index 385ee54..0fcaf8e 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -8,7 +8,7 @@
> */
>
> /*
> - * Copyright (C) 2010-2013 Red Hat, Inc.
> + * Copyright (C) 2010-2014 Red Hat, Inc.
> * Copyright (C) 2008-2009 Sun Microsystems, Inc.
> *
> * This file is part of a free software library; you can redistribute
> @@ -7284,10 +7284,12 @@ static void vboxReadCallback(int watch ATTRIBUTE_UNUSED,
> }
> }
>
> -static int vboxConnectDomainEventRegister(virConnectPtr conn,
> - virConnectDomainEventCallback callback,
> - void *opaque,
> - virFreeCallback freecb) {
> +static int
> +vboxConnectDomainEventRegister(virConnectPtr conn,
> + virConnectDomainEventCallback callback,
> + void *opaque,
> + virFreeCallback freecb)
> +{
> VBOX_OBJECT_CHECK(conn, int, -1);
> int vboxRet = -1;
Something can be tided up later.
> nsresult rc;
> @@ -7338,7 +7340,7 @@ static int vboxConnectDomainEventRegister(virConnectPtr conn,
> vboxDriverUnlock(data);
>
> if (ret >= 0) {
> - return ret;
> + return 0;
> } else {
> if (data->vboxObj && data->vboxCallback) {
> data->vboxObj->vtbl->UnregisterCallback(data->vboxObj, data->vboxCallback);
> @@ -7347,8 +7349,10 @@ static int vboxConnectDomainEventRegister(virConnectPtr conn,
> }
> }
>
> -static int vboxConnectDomainEventDeregister(virConnectPtr conn,
> - virConnectDomainEventCallback callback) {
> +static int
> +vboxConnectDomainEventDeregister(virConnectPtr conn,
> + virConnectDomainEventCallback callback)
> +{
> VBOX_OBJECT_CHECK(conn, int, -1);
> int cnt;
>
> @@ -7371,6 +7375,9 @@ static int vboxConnectDomainEventDeregister(virConnectPtr conn,
>
> vboxDriverUnlock(data);
>
> + if (cnt >= 0)
> + ret = 0;
This actually fixes bug, otherwise it always returns -1.
> +
> return ret;
> }
>
> @@ -7441,8 +7448,10 @@ static int vboxConnectDomainEventRegisterAny(virConnectPtr conn,
> }
> }
>
> -static int vboxConnectDomainEventDeregisterAny(virConnectPtr conn,
> - int callbackID) {
> +static int
> +vboxConnectDomainEventDeregisterAny(virConnectPtr conn,
> + int callbackID)
> +{
> VBOX_OBJECT_CHECK(conn, int, -1);
> int cnt;
>
> @@ -7465,6 +7474,9 @@ static int vboxConnectDomainEventDeregisterAny(virConnectPtr conn,
>
> vboxDriverUnlock(data);
>
> + if (cnt >= 0)
> + ret = 0;
Like above.
The changes themselvs are just sane.
Assuming that the application followed the documentation instead of the
code.
And since either changing the doc or the code could be able break something,
changing the code couldn't be worse than changing the doc. Well, who
knows. :-)
ACK.
Osier
More information about the libvir-list
mailing list