On 02/02/2012 11:22 AM, D. Herrendoerfer wrote:
From: "D. Herrendoerfer"<d.herrendoerfer@xxxxxxxxxxxxxxxxxx>
This is the second version of the netlink event code. it has most of
the proposed changes by Eric and Daniel included.
(actually Laine and Daniel :-)
From: "D. Herrendoerfer"<d.herrendoerfer@xxxxxxxxxxxxxxxxxx>
This code adds an event service for netlink messages addressed
to libvirt and passes the message to registered callback handlers.
Itself, it makes use of the polling file event service and follows
a similar design.
Signed-off-by: D. Herrendoerfer<d.herrendoerfer@xxxxxxxxxxxxxxxxxx>
---
daemon/libvirtd.c | 9 +
src/Makefile.am | 1 +
src/libvirt_private.syms | 8 +
src/util/virnetlink.c | 447 ++++++++++++++++++++++++++++++++++++++++++++++
src/util/virnetlink.h | 64 +++++++
5 files changed, 529 insertions(+), 0 deletions(-)
create mode 100644 src/util/virnetlink.c
create mode 100644 src/util/virnetlink.h
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index b1b542b..37fe32b 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -55,6 +55,8 @@
#include "uuid.h"
#include "viraudit.h"
+#include "virnetlink.h"
+
#ifdef WITH_DRIVER_MODULES
# include "driver.h"
#else
@@ -1598,6 +1600,12 @@ int main(int argc, char **argv) {
goto cleanup;
}
+ /* Register the netlink event service */
+ if (virNetlinkEventServiceStart()< 0) {
+ ret = VIR_DAEMON_ERR_NETWORK;
+ goto cleanup;
+ }
+
/* Run event loop. */
virNetServerRun(srv);
@@ -1607,6 +1615,7 @@ int main(int argc, char **argv) {
0, "shutdown", NULL);
cleanup:
+ virNetlinkEventServiceStop();
virNetServerProgramFree(remoteProgram);
virNetServerProgramFree(qemuProgram);
virNetServerClose(srv);
diff --git a/src/Makefile.am b/src/Makefile.am
index 4a5d9f7..65e4f27 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -66,6 +66,7 @@ UTIL_SOURCES = \
util/dnsmasq.c util/dnsmasq.h \
util/json.c util/json.h \
util/logging.c util/logging.h \
+ util/virnetlink.c util/virnetlink.h \
util/memory.c util/memory.h \
util/netlink.c util/netlink.h \
util/pci.c util/pci.h \
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ce71d8b..33930af 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -737,6 +737,14 @@ virShrinkN;
nlComm;
+#virnetlink.h
+virNetlinkEventServiceIsRunning;
+virNetlinkEventServiceStop;
+virNetlinkEventServiceStart;
+virNetlinkEventAddClient;
+virNetlinkEventRemoveClient;
+
+
# netdev_bandwidth_conf.h
virNetDevBandwidthFormat;
virNetDevBandwidthParse;
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
new file mode 100644
index 0000000..37bf0ef
--- /dev/null
+++ b/src/util/virnetlink.c
@@ -0,0 +1,447 @@
+/*
+ * virnetlink.c: event loop for monitoring netlink messages
+ *
+ * Copyright (C) 2011-2012 IBM Corporation.
+ * Copyright (C) 2011-2012 Dirk Herrendoerfer
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Author: Dirk Herrendoerfer<herrend[at]de[dot]ibm[dot]com>
+ */
+
+#include<config.h>
+
+#include<asm/types.h>
+#include<sys/socket.h>
+#include<netlink/netlink.h>
This would fail to compile on a system that didn't have netlink
installed. You should include "netlink.h" here instead - this is a file
in src/util that only includes <netlink/msg.h> (which itself includes
<netlink/netlink.h> if HAVE_LIBNL is defined.)
Actually, now that I've noticed that file, I actually think
util/netlink.[ch] should be merged into your util/virnetlink.[ch]. The
functions in both are related to netlink, and the name virnetlink fits
better with the naming scheme. This should probably be done by first
renaming netlink.[ch] to virnetlink.[ch] (and at the same time renaming
the nlComm() function to virNetlinkCommand() to be more consistent with
the naming scheme), then adding your functions into that file. (sorry
for not noticing this before).
+
+#include<errno.h>
+#include<unistd.h>
+#include<sys/types.h>
+
+#include "event.h"
+#include "logging.h"
+#include "memory.h"
+#include "netlink.h"
+#include "threads.h"
+#include "virmacaddr.h"
+#include "virnetlink.h"
+#include "virterror_internal.h"
+
+#define VIR_FROM_THIS VIR_FROM_NET
+#define virNetError(code, ...) \
+ virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \
+ __FUNCTION__, __LINE__, __VA_ARGS__)
+
+#if defined(__linux__)&& defined(HAVE_LIBNL)
+
+/* State for a single netlink event handle */
+struct virNetlinkEventHandle {
+ int watch;
+ virNetlinkEventHandleCallback cb;
+ void *opaque;
+ unsigned char macaddr[VIR_MAC_BUFLEN];
+ int deleted;
+};
+
+/* State for the main netlink event loop */
+struct virNetlinkEventLoop {
+ virMutex lock;
+ int handled;
+ size_t handlesCount;
+ size_t handlesAlloc;
+ struct virNetlinkEventHandle *handles;
+};
+
+typedef struct _virNetlinkEventSrvPrivate virNetlinkEventSrvPrivate;
+typedef virNetlinkEventSrvPrivate *virNetlinkEventSrvPrivatePtr;
+struct _virNetlinkEventSrvPrivate {
+ virMutex lock;
+ int eventwatch;
+ int netlinkfd;
+ struct nl_handle *netlinknh;
+};
+
+/* Only have one event loop */
+static struct virNetlinkEventLoop eventLoop;
+
+/* Unique ID for the next netlink watch to be registered */
+static int nextWatch = 1;
+
+/* Allocate extra slots for virEventPollHandle/virEventPollTimeout
+ records in this multiple */
+#define NETLINK_EVENT_ALLOC_EXTENT 10
+
+static virNetlinkEventSrvPrivatePtr server = 0;
+
+/* Function definitions */
+static void
+virNetlinkEventServerLock(virNetlinkEventSrvPrivatePtr driver) {
+ virMutexLock(&driver->lock);
+}
+
+static void
+virNetlinkEventServerUnlock(virNetlinkEventSrvPrivatePtr driver) {
+ virMutexUnlock(&driver->lock);
+}
+
+static void
+virNetlinkEventCallback(int watch,
+ int fd ATTRIBUTE_UNUSED,
+ int events ATTRIBUTE_UNUSED,
+ void *opaque) {
+ virNetlinkEventSrvPrivatePtr srv = opaque;
+ unsigned char *msg;
+ struct sockaddr_nl peer;
+ struct ucred *creds = NULL;
+ int i, length, handled;
+
+ length = nl_recv(srv->netlinknh,&peer,&msg,&creds);
+
+ virNetlinkEventServerLock(srv);
+
+ handled=0;
+
+ virMutexLock(&eventLoop.lock);
+
+ VIR_DEBUG("dispatching to max %d clients, called from event watch %d",
+ (int)eventLoop.handlesCount, watch);
+
+ for (i = 0 ; i< eventLoop.handlesCount ; i++) {
+ if (eventLoop.handles[i].deleted) {
+ continue;
+ }
+
+ VIR_DEBUG("dispatching client %d.",i);
+
+ virNetlinkEventHandleCallback cb = eventLoop.handles[i].cb;
+ void *cpopaque = eventLoop.handles[i].opaque;
+ (cb)( msg, length,&peer,&handled, cpopaque);
Is there ever any chance that cb could be NULL?
+ }
+
+ virMutexUnlock(&eventLoop.lock);
+
+ if (handled == 0) {
+ VIR_DEBUG("nobody cared.");
+ }
+
+ VIR_FREE(msg);
+
+ for (i = 0 ; i< eventLoop.handlesCount ; i++) {
+ if (eventLoop.handles[i].deleted == 1) {
+ VIR_FREE(eventLoop.handles[i].opaque);
+ eventLoop.handles[i].deleted = 2;
+ }
+ }
+ virNetlinkEventServerUnlock(srv);
+}
+
+static int
+setupNetlinkEventServer(virNetlinkEventSrvPrivatePtr srv) {
+ int fd;
This function never initializes either of the mutexes used by the
service. It should call virMutexInit() for each mutex (and be careful
not to call virMutex(Lock|Unlock|Destroy) in the cleanup path if that
fails).
+
+ virNetlinkEventServerLock(srv);
+
+ /* Allocate a new socket and get fd */
+ srv->netlinknh = nl_handle_alloc();
+
+ if (!srv->netlinknh) {
+ virNetError(errno,
+ "%s", _("cannot allocate nlhandle for virNetlinkEvent server"));
+ return -1;
+ }
+
+ if (nl_connect(srv->netlinknh, NETLINK_ROUTE)< 0) {
+ virNetError(errno,
+ "%s", _("cannot connect to netlink socket"));
+ goto exit_cleanup;
+ }
+
+ fd = nl_socket_get_fd(srv->netlinknh);
+ nl_socket_set_nonblocking(srv->netlinknh);
+
+ if ((srv->eventwatch = virEventAddHandle(fd,
+ VIR_EVENT_HANDLE_READABLE,
+ virNetlinkEventCallback,
+ srv, NULL))< 0) {
+ virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Failed to add netlink event handle watch"));
+
+ goto exit_cleanup;
+ }
+
+ srv->netlinkfd = fd;
+ VIR_DEBUG("netlink event listener on fd: %i",fd);
+
+ virNetlinkEventServerUnlock(srv);
+ return 0;
+
+exit_cleanup:
+ nl_close(srv->netlinknh);
+ nl_handle_destroy(srv->netlinknh);
I notice there are TAB characters at several places in your patch. the
libvirt standards are that all indentation is done with spaces, not
TABS. Please configure your editor to use spaces (and run "make
syntax-check" before submission).
+ virNetlinkEventServerUnlock(srv);
+ return -1;
+}
+
+/**
+ * virNetlinkEventServiceStop:
+ *
+ * stop the monitor to receive netlink messages for libvirtd.
+ * This removes the netlink socket fd from the event handler.
+ *
+ * returns -1 if the monitor cannot be unregistered, 0 upon success
+ */
+int
+virNetlinkEventServiceStop(void) {
+ virNetlinkEventSrvPrivatePtr srv = server;
+
+ VIR_INFO("stopping netlink event service");
+
+ if (!server) {
+ return 0;
+ }
+
+ virNetlinkEventServerLock(srv);
+
+ nl_close(srv->netlinknh);
+ nl_handle_destroy(srv->netlinknh);
+
+ virEventRemoveHandle(srv->eventwatch);
+ server=0;
+
+ virNetlinkEventServerUnlock(srv);
+ return 0;
+}
+
+/**
+ * virNetlinkEventServiceIsRunning:
+ *
+ * returns if the netlink event service is running.
+ *
+ * returns 1 if the is running, 0 if stopped.
+ */
+int
+virNetlinkEventServiceIsRunning(void) {
+ if (server)
+ return 1;
+ return 0;
+}
+
+/**
+ * virNetlinkEventServiceStart:
+ *
+ * start a monitor to receive netlink messages for libvirtd.
+ * This registers a netlink socket with the event interface.
+ *
+ * returns -1 if the monitor cannot be registered, 0 upon success
+ */
+int
+virNetlinkEventServiceStart(void) {
+ virNetlinkEventSrvPrivatePtr srv;
+
+ if (server) {
+ return 0;
+ }
+
+ VIR_INFO("starting netlink event service");
+
+ if (VIR_ALLOC(srv)< 0)
+ goto no_memory;
+
+ if (setupNetlinkEventServer(srv)) {
If the setup fails, you've leaked srv - you need to free it.
+ goto error;
+ }
+
+ VIR_DEBUG("netlink event service running");
+
+ server=srv;
+ return 0;
+
+ no_memory:
+ virReportOOMError();
+ error:
+ return -1;
+}
+
+/**
+ * virNetlinkEventAddClient:
+ *
+ * @cb: callback to invoke when an event occurs
+ * @opaque: user data to pass to callback
+ * @macaddr: macaddr to store with the data. Used to identify callers. May be null.
+ *
+ * register a callback for handling of netlink messages. The
+ * registered function receives the entire netlink message and
+ * may choose to act upon it.
+ *
+ * returns -1 if the file handle cannot be registered, number of monitor upon success
+ */
+int
+virNetlinkEventAddClient(virNetlinkEventHandleCallback cb,
+ void *opaque,
+ const unsigned char *macaddr) {
+ int i,r, result;
+
+ virMutexLock(&eventLoop.lock);
+
+ VIR_DEBUG("adding client: %d.",nextWatch);
+
+ r = 0;
+ /* first try to re-use deleted free slots */
+ for (i = 0 ; i< eventLoop.handlesCount ; i++) {
+ if (eventLoop.handles[i].deleted == 2) {
It looks like a value of 1 means "in the process of deleting", and 2
means "deleted and available". Rather than having magic values, I think
it would be better to have an enum for this. Also, I notice that an
entry will only move from "deleting" to "available" if there is a
netlink event. So if there are no events, the entry would never become
available again. Can you maybe force an event after you've marked an
entry as "deleting"? Or maybe your existing locks are enough that you
don't even need to do that, just have bool deleted and set it directly
from false to true in the RemoveClient function.
+ r = i;
+ goto addentry;
+ }
+ }
+ /* Resize the eventLoop array if needed */
+ if (eventLoop.handlesCount == eventLoop.handlesAlloc) {
+ VIR_DEBUG("Used %zu handle slots, adding at least %d more",
+ eventLoop.handlesAlloc, NETLINK_EVENT_ALLOC_EXTENT);
+ if (VIR_RESIZE_N(eventLoop.handles, eventLoop.handlesAlloc,
+ eventLoop.handlesCount, NETLINK_EVENT_ALLOC_EXTENT)< 0) {
+ virMutexUnlock(&eventLoop.lock);
+ return -1;
+ }
+ }
+ r = eventLoop.handlesCount;
+
+addentry:
+ eventLoop.handles[r].watch = nextWatch;
+ eventLoop.handles[r].cb = cb;
+ eventLoop.handles[r].opaque = opaque;
+ eventLoop.handles[r].deleted = 0;
+ if (macaddr)
+ memcpy(eventLoop.handles[r].macaddr, macaddr,VIR_MAC_BUFLEN);
+
+ VIR_DEBUG("added client to loop slot: %d.",(int)eventLoop.handlesCount);
+
+ eventLoop.handlesCount++;
You've just increased the total handlesCount even if you had found an
empty entry in the middle. Instead, you should remove the increment
here, and change the line before addentry: to "r = eventLoop.handlesCount++;
+ result = nextWatch++;
+ virMutexUnlock(&eventLoop.lock);
+
+ return result;
+}
+
+/**
+ * virNetlinkEventRemoveClient:
+ *
+ * @watch: watch whose handle to remove
+ * @macaddr: macaddr whose handle to remove
+ *
+ * Unregister a callback from a netlink monitor.
+ * The handler function referenced will no longer receive netlink messages.
+ * Either watch or macaddr may be used, the other should be null.
+ *
+ * returns -1 if the file handle was not registered, 0 upon success
+ */
+int
+virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr) {
+ int i;
+
+ if (watch<= 0&& macaddr == 0) {
+ VIR_WARN("Ignoring invalid netlink client id: %d", watch);
+ return -1;
+ }
+
+ virMutexLock(&eventLoop.lock);
+ for (i = 0 ; i< eventLoop.handlesCount ; i++) {
+ if (eventLoop.handles[i].deleted)
+ continue;
+
+ if (watch != 0&& eventLoop.handles[i].watch == watch) {
+ eventLoop.handles[i].deleted = 1;
+ virMutexUnlock(&eventLoop.lock);
+ VIR_DEBUG("removed client: %d by index.",
+ eventLoop.handles[i].watch);
+ return 0;
+ }
+ if (watch == 0&& memcmp(macaddr, eventLoop.handles[i].macaddr, VIR_MAC_BUFLEN)) {
+ eventLoop.handles[i].deleted = 1;
+ virMutexUnlock(&eventLoop.lock);
+ VIR_DEBUG("removed client: %d by mac.",
+ eventLoop.handles[i].watch);
+ return 0;
+ }
+ }
Multiple returns in a function, and multiple Unlocks, always make me
nervous. How about initializing an int ret = -1, then setting it to 0 if
either watch or macaddr is matched, then breaking out of the loop early
and doing "if (found) { ..... } else { }; followed by a single
virMutexUnlock, and "return ret;"?
+ virMutexUnlock(&eventLoop.lock);
+
+ VIR_DEBUG("client not found to remove.");
+ return -1;
+}
+
+#else
+
+/**
+ * stopNetlinkEventServer: stop the monitor to receive netlink messages for libvirtd
+ */
+int virNetlinkEventServiceStop(void) {
+ virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
+# if defined(__linux__)&& !defined(HAVE_LIBNL)
+ _("virNetlinkEventServiceStop is not supported since libnl was not available"));
+# endif
+ return 0;
+
+}
+
+/**
+ * startNetlinkEventServer: start a monitor to receive netlink messages for libvirtd
+ */
+int virNetlinkEventServiceStart(void) {
+ virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
+# if defined(__linux__)&& !defined(HAVE_LIBNL)
+ _("virNetlinkEventServiceStart is not supported since libnl was not available"));
+# endif
+ return 0;
+}
+
+/**
+ * virNetlinkEventServiceIsRunning: returns if the netlink event service is running.
+ */
+int virNetlinkEventServiceIsRunning(void) {
+ virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
+# if defined(__linux__)&& !defined(HAVE_LIBNL)
+ _("virNetlinkEventServiceIsRunning is not supported since libnl was not available"));
+# endif
+ return 0;
+}
+
+/**
+ * virNetlinkEventAddClient: register a callback for handling of netlink messages
+ */
+int virNetlinkEventAddClient(virNetlinkEventHandleCallback cb, void *opaque, const unsigned char *macaddr) {
+ virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
+# if defined(__linux__)&& !defined(HAVE_LIBNL)
+ _("virNetlinkEventServiceAddClient is not supported since libnl was not available"));
+# else
+ _("virNetlinkEventServiceAddClient is not supported on non-linux platforms"));
+# endif
+ return -1;
+}
+
+/**
+ * virNetlinkEventRemoveClient: unregister a callback from a netlink monitor
+ */
+int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr) {
+ virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
+# if defined(__linux__)&& !defined(HAVE_LIBNL)
+ _("virNetlinkEventRemoveClient is not supported since libnl was not available"));
+# else
+ _("virNetlinkEventRemoveClient is not supported on non-linux platforms"));
+# endif
+ return -1;
+}
+
+#endif // __linux__&& HAVE_LIBNL
diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h
new file mode 100644
index 0000000..25e2636
--- /dev/null
+++ b/src/util/virnetlink.h
@@ -0,0 +1,64 @@
+/*
+ * virnetlink.h: event loop for monitoring netlink messages
+ *
+ * Copyright (C) 2011-2012 IBM Corporation.
+ * Copyright (C) 2011-2012 Dirk Herrendoerfer
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Author: Dirk Herrendoerfer<herrend[at]de[dot]ibm[dot]com>
+ */
+
+#ifndef NETLINK_EVENT_CONF_H
+# define NETLINK_EVENT_CONF_H
+
+# if defined(__linux__)&& defined(HAVE_LIBNL)
+
+# include<netlink/netlink.h>
+
+# else
+ struct sockaddr_nl;
+# endif // __linux__&& HAVE_LIBNL
+
+#include "internal.h"
+
+typedef void (*virNetlinkEventHandleCallback)( unsigned char *msg, int length, struct sockaddr_nl *peer, int *handled, void *opaque);
+
+/**
+ * stopNetlinkEventServer: stop the monitor to receive netlink messages for libvirtd
+ */
+int virNetlinkEventServiceStop(void);
+
+/**
+ * startNetlinkEventServer: start a monitor to receive netlink messages for libvirtd
+ */
+int virNetlinkEventServiceStart(void);
+
+/**
+ * virNetlinkEventServiceIsRunning: returns if the netlink event service is running.
+ */
+int virNetlinkEventServiceIsRunning(void);
+
+/**
+ * virNetlinkEventAddClient: register a callback for handling of netlink messages
+ */
+int virNetlinkEventAddClient(virNetlinkEventHandleCallback cb, void *opaque, const unsigned char *macaddr);
+
+/**
+ * virNetlinkEventRemoveClient: unregister a callback from a netlink monitor
+ */
+int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr);
+
+#endif /* NETLINK_EVENT_CONF_H */
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list