[libvirt] [PATCH] event: don't overwrite registration error message

[Eric Blake]

Prior to this patch, an attempt to register an event without an
event loop started results in the vague:

libvirt: Remote Driver error : adding cb to list

Now it gives the much nicer:

libvirt: error : internal error: could not initialize domain event timer

This also avoids hiding other reasonable error messages, such as
attempts to register a duplicate callback or OOM errors.

Also, document the event loop usage requirement, since that was how
I ran into the issue.

* src/remote/remote_driver.c (remoteConnectNetworkEventRegisterAny)
(remoteConnectDomainEventRegister)
(remoteConnectDomainEventRegisterAny): Preserve more detailed error.
* src/libvirt.c (virConnectDomainEventRegister)
(virConnectDomainEventRegisterAny)
(virConnectNetworkEventRegisterAny): Document event loop requirement.

Signed-off-by: Eric Blake <***@redhat.com>
---
src/libvirt.c | 24 +++++++++++++++---------
src/remote/remote_driver.c | 12 +++---------
2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/src/libvirt.c b/src/libvirt.c
index 90773bb..f8b11b3 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -14829,11 +14829,13 @@ error:
* @freecb: optional function to deallocate opaque when not used anymore
*
* Adds a callback to receive notifications of domain lifecycle events
- * occurring on a connection
+ * occurring on a connection. This function requires that an event loop
+ * has been previously registered with virEventRegisterImpl() or
+ * virEventRegisterDefaultImpl().
*
* Use of this method is no longer recommended. Instead applications
* should try virConnectDomainEventRegisterAny() which has a more flexible
- * API contract
+ * API contract.
*
* The virDomainPtr object handle passed into the callback upon delivery
* of an event is only valid for the duration of execution of the callback.
@@ -14842,7 +14844,7 @@ 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.
*/
int
virConnectDomainEventRegister(virConnectPtr conn,
@@ -17541,10 +17543,12 @@ error:
* @freecb: optional function to deallocate opaque when not used anymore
*
* Adds a callback to receive notifications of arbitrary domain events
- * occurring on a domain.
+ * occurring on a domain. This function requires that an event loop
+ * has been previously registered with virEventRegisterImpl() or
+ * virEventRegisterDefaultImpl().
*
* If @dom is NULL, then events will be monitored for any domain. If @dom
- * is non-NULL, then only the specific domain will be monitored
+ * is non-NULL, then only the specific domain will be monitored.
*
* Most types of event have a callback providing a custom set of parameters
* for the event. When registering an event, it is thus necessary to use
@@ -17562,7 +17566,7 @@ error:
* for the callback. To unregister a callback, this callback ID should
* be passed to the virConnectDomainEventDeregisterAny() method.
*
- * Returns a callback identifier on success, -1 on failure
+ * Returns a callback identifier on success, -1 on failure.
*/
int
virConnectDomainEventRegisterAny(virConnectPtr conn,
@@ -17657,10 +17661,12 @@ error:
* @freecb: optional function to deallocate opaque when not used anymore
*
* Adds a callback to receive notifications of arbitrary network events
- * occurring on a network.
+ * occurring on a network. This function requires that an event loop
+ * has been previously registered with virEventRegisterImpl() or
+ * virEventRegisterDefaultImpl().
*
* If @net is NULL, then events will be monitored for any network. If @net
- * is non-NULL, then only the specific network will be monitored
+ * is non-NULL, then only the specific network will be monitored.
*
* Most types of event have a callback providing a custom set of parameters
* for the event. When registering an event, it is thus necessary to use
@@ -17678,7 +17684,7 @@ error:
* for the callback. To unregister a callback, this callback ID should
* be passed to the virConnectNetworkEventDeregisterAny() method.
*
- * Returns a callback identifier on success, -1 on failure
+ * Returns a callback identifier on success, -1 on failure.
*/
int
virConnectNetworkEventRegisterAny(virConnectPtr conn,
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index be282d6..11785e2 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -2932,10 +2932,8 @@ remoteConnectNetworkEventRegisterAny(virConnectPtr conn,
net, eventID,
VIR_OBJECT_EVENT_CALLBACK(callback),
opaque, freecb,
- &callbackID)) < 0) {
- virReportError(VIR_ERR_RPC, "%s", _("adding cb to list"));
+ &callbackID)) < 0)
goto done;
- }

/* If this is the first callback for this eventID, we need to enable
* events on the server */
@@ -4424,10 +4422,8 @@ static int remoteConnectDomainEventRegister(virConnectPtr conn,
remoteDriverLock(priv);

if ((count = virDomainEventStateRegister(conn, priv->domainEventState,
- callback, opaque, freecb)) < 0) {
- virReportError(VIR_ERR_RPC, "%s", _("adding cb to list"));
+ callback, opaque, freecb)) < 0)
goto done;
- }

if (count == 1) {
/* Tell the server when we are the first callback deregistering */
@@ -5234,10 +5230,8 @@ static int remoteConnectDomainEventRegisterAny(virConnectPtr conn,
priv->domainEventState,
dom, eventID,
callback, opaque, freecb,
- &callbackID)) < 0) {
- virReportError(VIR_ERR_RPC, "%s", _("adding cb to list"));
+ &callbackID)) < 0)
goto done;
- }

/* If this is the first callback for this eventID, we need to enable
* events on the server */

[Martin Kletzander]

Post by Eric Blake
Prior to this patch, an attempt to register an event without an
libvirt: Remote Driver error : adding cb to list
libvirt: error : internal error: could not initialize domain event timer
This also avoids hiding other reasonable error messages, such as
attempts to register a duplicate callback or OOM errors.
Also, document the event loop usage requirement, since that was how
I ran into the issue.
* src/remote/remote_driver.c (remoteConnectNetworkEventRegisterAny)
(remoteConnectDomainEventRegister)
(remoteConnectDomainEventRegisterAny): Preserve more detailed error.
* src/libvirt.c (virConnectDomainEventRegister)
(virConnectDomainEventRegisterAny)
(virConnectNetworkEventRegisterAny): Document event loop requirement.
---
src/libvirt.c | 24 +++++++++++++++---------
src/remote/remote_driver.c | 12 +++---------
2 files changed, 18 insertions(+), 18 deletions(-)

ACK, although I wasn't (somehow) able to test it with python bindings.

Martin

[Eric Blake]

Post by Martin Kletzander

Post by Eric Blake
Prior to this patch, an attempt to register an event without an
libvirt: Remote Driver error : adding cb to list
libvirt: error : internal error: could not initialize domain event timer
This also avoids hiding other reasonable error messages, such as
attempts to register a duplicate callback or OOM errors.
Also, document the event loop usage requirement, since that was how
I ran into the issue.

I actually moved the doc changes into a separate patch, since they
turned out to be bigger in nature:
https://www.redhat.com/archives/libvir-list/2013-December/msg01334.html

Post by Martin Kletzander
ACK, although I wasn't (somehow) able to test it with python bindings.

Thanks for the review. It turns out reproducing this in python is
fairly straightforward; here run pre-patch:

$ python
... print('hi')
...

Post by Martin Kletzander

Post by Eric Blake

import libvirt
conn = libvirt.open('qemu:///system')
conn.domainEventRegisterAny(None, 0, hi, '')

libvirt: Remote Driver error : adding cb to list
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib64/python2.7/site-packages/libvirt.py", line 3865, in
domainEventRegisterAny
raise libvirtError ('virConnectDomainEventRegisterAny() failed',
conn=self)
libvirt.libvirtError: adding cb to list

Not that I'm sure how to set up the python event loop, but that's one
way to see the improved error message.

At any rate, I've pushed the patch (while waiting for the doc patch review).

[Martin Kletzander]

Post by Eric Blake

Post by Martin Kletzander

Post by Eric Blake
Prior to this patch, an attempt to register an event without an
libvirt: Remote Driver error : adding cb to list
libvirt: error : internal error: could not initialize domain event timer
This also avoids hiding other reasonable error messages, such as
attempts to register a duplicate callback or OOM errors.
Also, document the event loop usage requirement, since that was how
I ran into the issue.

I actually moved the doc changes into a separate patch, since they
https://www.redhat.com/archives/libvir-list/2013-December/msg01334.html

Post by Martin Kletzander
ACK, although I wasn't (somehow) able to test it with python bindings.

Thanks for the review. It turns out reproducing this in python is

I was able to reproduce it, but I wasn't able to test it after I
merged the fix. I'm probably misusing the python bindings (still
didn't get used to the workflow when they are split).

Post by Eric Blake
$ python
... print('hi')
...

Post by Martin Kletzander

Post by Eric Blake

import libvirt
conn = libvirt.open('qemu:///system')
conn.domainEventRegisterAny(None, 0, hi, '')

libvirt: Remote Driver error : adding cb to list
File "<stdin>", line 1, in <module>
File "/usr/lib64/python2.7/site-packages/libvirt.py", line 3865, in
domainEventRegisterAny
raise libvirtError ('virConnectDomainEventRegisterAny() failed',
conn=self)
libvirt.libvirtError: adding cb to list
Not that I'm sure how to set up the python event loop, but that's one
way to see the improved error message.
At any rate, I've pushed the patch (while waiting for the doc patch review).

Getting right on that.

Martin