Re: [libvirt] [PATCH v4 4/7] Add network events to the remote driver
On 01/09/2014 07:50 PM, Eric Blake wrote: > On 12/12/2013 07:30 AM, Cedric Bosdonnat wrote: >> Hi John, >> > +event = virNetworkEventLifecycleNew(net->name, net->uuid, msg->event); +virNetworkFree(net); +remoteDomainEventQueue(priv, event); > >>> Essentially - you need to check for NULL event. >> >> Sure, but the weird thing it that it should also complain on the similar >> functions for domain events, right above this one in the same file. > > Revisiting this. The problem can only happen on OOM situations, so it's > not that likely to hit in real life; but we should indeed fix our code. > I think the easiest way is to fix remoteDomainEventQueue to gracefully > do nothing on a NULL event (events are best effort, and if OOM occurs, > skipping event delivery is an acceptable action, since we have no way of > reporting the error). I'll post a patch. Actually, it turns out we are quite lucky - appending a NULL event to the queue had no ill effects, because when checking for what callbacks to dispatch, our first use of events was an early exit if (!virObjectIsClass(event, cb->klass)). So the ATTRIBUTE_NONNULL could actually be deleted - but that in turn implies we have a lot of code that checks for non-null events before queueing them up which could also be simplified. And even if we did that, queueing a NULL event is a waste of resources compared to just avoiding them altogether. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 4/7] Add network events to the remote driver
On 12/12/2013 07:30 AM, Cedric Bosdonnat wrote: > Hi John, > >>> +event = virNetworkEventLifecycleNew(net->name, net->uuid, msg->event); >>> +virNetworkFree(net); >>> +remoteDomainEventQueue(priv, event); >> Essentially - you need to check for NULL event. > > Sure, but the weird thing it that it should also complain on the similar > functions for domain events, right above this one in the same file. Revisiting this. The problem can only happen on OOM situations, so it's not that likely to hit in real life; but we should indeed fix our code. I think the easiest way is to fix remoteDomainEventQueue to gracefully do nothing on a NULL event (events are best effort, and if OOM occurs, skipping event delivery is an acceptable action, since we have no way of reporting the error). I'll post a patch. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 4/7] Add network events to the remote driver
Hi John, On Thu, 2013-12-12 at 06:50 -0500, John Ferlan wrote: > 4920 > > (3) Event returned_null: Function "virNetworkEventLifecycleNew(char > const *, unsigned char const *, int)" returns null (checked 10 out of 11 > times). [details] > (14) Event var_assigned: Assigning: "event" = null return value from > "virNetworkEventLifecycleNew(char const *, unsigned char const *, int)". > Also see events: > [example_assign][example_checked][example_assign][example_checked][example_assign][example_checked][example_assign][example_checked][example_assign][example_checked][dereference] > > 4921 event = virNetworkEventLifecycleNew(net->name, net->uuid, > msg->event); > > > +event = virNetworkEventLifecycleNew(net->name, net->uuid, msg->event); > > +virNetworkFree(net); > > + > > (15) Event dereference: Dereferencing a pointer that might be null > "event" when calling "remoteDomainEventQueue(struct private_data *, > virObjectEventPtr)". [details] > Also see events: > [returned_null][example_assign][example_checked][example_assign][example_checked][example_assign][example_checked][example_assign][example_checked][example_assign][example_checked][var_assigned] > > > > +remoteDomainEventQueue(priv, event); > > +} > > + > > + > > Essentially - you need to check for NULL event. Sure, but the weird thing it that it should also complain on the similar functions for domain events, right above this one in the same file. -- Cedric -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 4/7] Add network events to the remote driver
My overnight Coverity run found an issue in the function:
'remoteNetworkBuildEventLifecycle'
On 12/11/2013 05:38 AM, Cédric Bosdonnat wrote:
> ---
> daemon/libvirtd.h| 1 +
> daemon/remote.c | 139
> +++
> src/remote/remote_driver.c | 127 +++
> src/remote/remote_protocol.x | 46 +-
> 4 files changed, 312 insertions(+), 1 deletion(-)
>
<...snip...>
>
> +static void
> +remoteNetworkBuildEventLifecycle(virNetClientProgramPtr prog
> ATTRIBUTE_UNUSED,
> + virNetClientPtr client ATTRIBUTE_UNUSED,
> + void *evdata, void *opaque)
> +{
> +virConnectPtr conn = opaque;
> +struct private_data *priv = conn->privateData;
> +remote_network_event_lifecycle_msg *msg = evdata;
> +virNetworkPtr net;
> +virObjectEventPtr event = NULL;
> +
> +net = get_nonnull_network(conn, msg->net);
> +if (!net)
> +return;
> +
4920
(3) Event returned_null:Function "virNetworkEventLifecycleNew(char
const *, unsigned char const *, int)" returns null (checked 10 out of 11
times). [details]
(14) Event var_assigned:Assigning: "event" = null return value from
"virNetworkEventLifecycleNew(char const *, unsigned char const *, int)".
Also see events:
[example_assign][example_checked][example_assign][example_checked][example_assign][example_checked][example_assign][example_checked][example_assign][example_checked][dereference]
4921event = virNetworkEventLifecycleNew(net->name, net->uuid,
msg->event);
> +event = virNetworkEventLifecycleNew(net->name, net->uuid, msg->event);
> +virNetworkFree(net);
> +
(15) Event dereference: Dereferencing a pointer that might be null
"event" when calling "remoteDomainEventQueue(struct private_data *,
virObjectEventPtr)". [details]
Also see events:
[returned_null][example_assign][example_checked][example_assign][example_checked][example_assign][example_checked][example_assign][example_checked][example_assign][example_checked][var_assigned]
> +remoteDomainEventQueue(priv, event);
> +}
> +
> +
Essentially - you need to check for NULL event.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 4/7] Add network events to the remote driver
On Wed, Dec 11, 2013 at 11:38:01AM +0100, Cédric Bosdonnat wrote: > --- > daemon/libvirtd.h| 1 + > daemon/remote.c | 139 > +++ > src/remote/remote_driver.c | 127 +++ > src/remote/remote_protocol.x | 46 +- > 4 files changed, 312 insertions(+), 1 deletion(-) Oppps, missed changes to src/remote_protocol-structs which causes 'make check' to fail - I've added the obvious bits. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 4/7] Add network events to the remote driver
On Wed, Dec 11, 2013 at 11:38:01AM +0100, Cédric Bosdonnat wrote:
> ---
> daemon/libvirtd.h| 1 +
> daemon/remote.c | 139
> +++
> src/remote/remote_driver.c | 127 +++
> src/remote/remote_protocol.x | 46 +-
> 4 files changed, 312 insertions(+), 1 deletion(-)
>
> diff --git a/daemon/libvirtd.h b/daemon/libvirtd.h
> index d0afdc8..47f2589 100644
> --- a/daemon/libvirtd.h
> +++ b/daemon/libvirtd.h
> @@ -50,6 +50,7 @@ struct daemonClientPrivate {
> virMutex lock;
>
> int domainEventCallbackID[VIR_DOMAIN_EVENT_ID_LAST];
> +int networkEventCallbackID[VIR_NETWORK_EVENT_ID_LAST];
>
> # if WITH_SASL
> virNetSASLSessionPtr sasl;
> diff --git a/daemon/remote.c b/daemon/remote.c
> index f060006..bcd73d4 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -49,6 +49,7 @@
> #include "qemu_protocol.h"
> #include "lxc_protocol.h"
> #include "virstring.h"
> +#include "object_event.h"
>
> #define VIR_FROM_THIS VIR_FROM_RPC
>
> @@ -653,6 +654,38 @@ static virConnectDomainEventGenericCallback
> domainEventCallbacks[] = {
>
> verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST);
>
> +static int remoteRelayNetworkEventLifecycle(virConnectPtr conn
> ATTRIBUTE_UNUSED,
> + virNetworkPtr net,
> + int event,
> + int detail,
> + void *opaque)
Indent.
> @@ -5216,6 +5261,100 @@ cleanup:
> }
>
>
> +static int
> +remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server
> ATTRIBUTE_UNUSED,
> + virNetServerClientPtr client
> ATTRIBUTE_UNUSED,
> + virNetMessagePtr msg
> ATTRIBUTE_UNUSED,
> + virNetMessageErrorPtr rerr
> ATTRIBUTE_UNUSED,
> +
> remote_connect_network_event_register_any_args *args,
> +
> remote_connect_network_event_register_any_ret *ret ATTRIBUTE_UNUSED)
> +{
> +int callbackID;
> +int rv = -1;
> +struct daemonClientPrivate *priv =
> +virNetServerClientGetPrivateData(client);
> +
> +if (!priv->conn) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
> open"));
> +goto cleanup;
> +}
> +
> +virMutexLock(&priv->lock);
> +
> +if ((args->eventID & 0xFF) >= VIR_NETWORK_EVENT_ID_LAST) {
We can drop the "& 0xFF" bit now namespaces aren't visible on the wire
> +virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported network event
> ID %d"), args->eventID);
> +goto cleanup;
> +}
> +
> +if (priv->networkEventCallbackID[args->eventID] != -1) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, _("network event %d already
> registered"), args->eventID);
> +goto cleanup;
> +}
> +
> +if ((callbackID = virConnectNetworkEventRegisterAny(priv->conn,
> +NULL,
> +args->eventID,
> +
> networkEventCallbacks[args->eventID],
> +client, NULL)) < 0)
> +goto cleanup;
> +
> +priv->networkEventCallbackID[args->eventID & 0xFF] = callbackID;
And again here.
> +
> +rv = 0;
> +
> +cleanup:
> +if (rv < 0)
> +virNetMessageSaveError(rerr);
> +virMutexUnlock(&priv->lock);
> +return rv;
> +}
> +
> +
> +static int
> +remoteDispatchConnectNetworkEventDeregisterAny(virNetServerPtr server
> ATTRIBUTE_UNUSED,
> + virNetServerClientPtr client
> ATTRIBUTE_UNUSED,
> + virNetMessagePtr msg
> ATTRIBUTE_UNUSED,
> + virNetMessageErrorPtr rerr
> ATTRIBUTE_UNUSED,
> +
> remote_connect_network_event_deregister_any_args *args,
> +
> remote_connect_network_event_deregister_any_ret *ret ATTRIBUTE_UNUSED)
> +{
> +int callbackID = -1;
> +int rv = -1;
> +struct daemonClientPrivate *priv =
> +virNetServerClientGetPrivateData(client);
> +
> +if (!priv->conn) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
> open"));
> +goto cleanup;
> +}
> +
> +virMutexLock(&priv->lock);
> +
> +if ((args->eventID & 0xFF) >= VIR_NETWORK_EVENT_ID_LAST) {
And here
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index 4a84a52..046f424 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -2
[libvirt] [PATCH v4 4/7] Add network events to the remote driver
---
daemon/libvirtd.h| 1 +
daemon/remote.c | 139 +++
src/remote/remote_driver.c | 127 +++
src/remote/remote_protocol.x | 46 +-
4 files changed, 312 insertions(+), 1 deletion(-)
diff --git a/daemon/libvirtd.h b/daemon/libvirtd.h
index d0afdc8..47f2589 100644
--- a/daemon/libvirtd.h
+++ b/daemon/libvirtd.h
@@ -50,6 +50,7 @@ struct daemonClientPrivate {
virMutex lock;
int domainEventCallbackID[VIR_DOMAIN_EVENT_ID_LAST];
+int networkEventCallbackID[VIR_NETWORK_EVENT_ID_LAST];
# if WITH_SASL
virNetSASLSessionPtr sasl;
diff --git a/daemon/remote.c b/daemon/remote.c
index f060006..bcd73d4 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -49,6 +49,7 @@
#include "qemu_protocol.h"
#include "lxc_protocol.h"
#include "virstring.h"
+#include "object_event.h"
#define VIR_FROM_THIS VIR_FROM_RPC
@@ -653,6 +654,38 @@ static virConnectDomainEventGenericCallback
domainEventCallbacks[] = {
verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST);
+static int remoteRelayNetworkEventLifecycle(virConnectPtr conn
ATTRIBUTE_UNUSED,
+ virNetworkPtr net,
+ int event,
+ int detail,
+ void *opaque)
+{
+virNetServerClientPtr client = opaque;
+remote_network_event_lifecycle_msg data;
+
+if (!client)
+return -1;
+
+VIR_DEBUG("Relaying network lifecycle event %d, detail %d", event, detail);
+
+/* build return data */
+memset(&data, 0, sizeof(data));
+make_nonnull_network(&data.net, net);
+data.event = event;
+
+remoteDispatchObjectEventSend(client, remoteProgram,
+ REMOTE_PROC_NETWORK_EVENT_LIFECYCLE,
+
(xdrproc_t)xdr_remote_network_event_lifecycle_msg, &data);
+
+return 0;
+}
+
+static virConnectNetworkEventGenericCallback networkEventCallbacks[] = {
+VIR_NETWORK_EVENT_CALLBACK(remoteRelayNetworkEventLifecycle),
+};
+
+verify(ARRAY_CARDINALITY(networkEventCallbacks) == VIR_NETWORK_EVENT_ID_LAST);
+
/*
* You must hold lock for at least the client
* We don't free stuff here, merely disconnect the client's
@@ -680,6 +713,15 @@ void remoteClientFreeFunc(void *data)
priv->domainEventCallbackID[i] = -1;
}
+for (i = 0; i < VIR_NETWORK_EVENT_ID_LAST; i++) {
+if (priv->networkEventCallbackID[i] != -1) {
+VIR_DEBUG("Deregistering to relay remote events %zu", i);
+virConnectNetworkEventDeregisterAny(priv->conn,
+
priv->networkEventCallbackID[i]);
+}
+priv->networkEventCallbackID[i] = -1;
+}
+
virConnectClose(priv->conn);
virIdentitySetCurrent(NULL);
@@ -716,6 +758,9 @@ void *remoteClientInitHook(virNetServerClientPtr client,
for (i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i++)
priv->domainEventCallbackID[i] = -1;
+for (i = 0; i < VIR_NETWORK_EVENT_ID_LAST; i++)
+priv->networkEventCallbackID[i] = -1;
+
virNetServerClientSetCloseHook(client, remoteClientCloseFunc);
return priv;
}
@@ -5216,6 +5261,100 @@ cleanup:
}
+static int
+remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client
ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg
ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
+
remote_connect_network_event_register_any_args *args,
+
remote_connect_network_event_register_any_ret *ret ATTRIBUTE_UNUSED)
+{
+int callbackID;
+int rv = -1;
+struct daemonClientPrivate *priv =
+virNetServerClientGetPrivateData(client);
+
+if (!priv->conn) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
+goto cleanup;
+}
+
+virMutexLock(&priv->lock);
+
+if ((args->eventID & 0xFF) >= VIR_NETWORK_EVENT_ID_LAST) {
+virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported network event ID
%d"), args->eventID);
+goto cleanup;
+}
+
+if (priv->networkEventCallbackID[args->eventID] != -1) {
+virReportError(VIR_ERR_INTERNAL_ERROR, _("network event %d already
registered"), args->eventID);
+goto cleanup;
+}
+
+if ((callbackID = virConnectNetworkEventRegisterAny(priv->conn,
+NULL,
+args->eventID,
+
networkEventCallbacks[args
