On Jan 26, 2012, at 5:32 PM, Laine Stump wrote: On 01/20/2012 09:56 AM, D. Herrendoerfer wrote: 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/Makefile.am | 3 +- daemon/libvirtd.c | 7 + src/Makefile.am | 1 + src/libvirt_private.syms | 7 + src/util/netlink-event.c | 363 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/netlink-event.h | 63 ++++++++ src/util/netlink.c | 1 + 7 files changed, 444 insertions(+), 1 deletions(-) create mode 100644 src/util/netlink-event.c create mode 100644 src/util/netlink-event.h diff --git a/daemon/Makefile.am b/daemon/Makefile.am index 73a6e1f..d027ff6 100644 --- a/daemon/Makefile.am +++ b/daemon/Makefile.am @@ -114,7 +114,8 @@ libvirtd_LDADD += ../src/probes.o endif libvirtd_LDADD += \ - ../src/libvirt-qemu.la + ../src/libvirt-qemu.la \ + ../src/libvirt_util.la if ! WITH_DRIVER_MODULES if WITH_QEMU diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index d7a03d7..b118fd0 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -55,6 +55,8 @@ #include "uuid.h" #include "viraudit.h" +#include "netlink-event.h" + #ifdef WITH_DRIVER_MODULES # include "driver.h" #else @@ -1577,6 +1579,11 @@ int main(int argc, char **argv) { goto cleanup; } + /* Register the netlink event service */ + if (netlinkEventServiceStart()< 0) { + VIR_WARN("Netlink service did not start. Netlink events are not available."); + } + /* Run event loop. */ virNetServerRun(srv); You started your service, but never stop it when libvirtd is shutting down (even though you have a function to do that). diff --git a/src/Makefile.am b/src/Makefile.am index 93bf54c..abaeb9c 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -67,6 +67,7 @@ UTIL_SOURCES = \ util/dnsmasq.c util/dnsmasq.h \ util/json.c util/json.h \ util/logging.c util/logging.h \ + util/netlink-event.c util/netlink-event.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 48ffdf2..34a36bd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -743,6 +743,13 @@ virShrinkN; nlComm; +#netlink-event.h +netlinkEventServiceStop; +netlinkEventServiceStart; +netlinkEventAddClient; +netlinkEventRemoveClient; + + # netdev_bandwidth_conf.h virNetDevBandwidthFormat; virNetDevBandwidthParse; diff --git a/src/util/netlink-event.c b/src/util/netlink-event.c new file mode 100644 index 0000000..7c6746d --- /dev/null +++ b/src/util/netlink-event.c @@ -0,0 +1,363 @@ +/* + * lldpad-event.c: event loop for monitoring netlink messages name change ^^ All agreed. + * + * 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> + +#include<errno.h> +#include<unistd.h> +#include<sys/types.h> + +#include "event.h" +#include "logging.h" +#include "memory.h" +#include "netlink.h" +#include "netlink-event.h" +#include "virterror_internal.h" + +#define VIR_FROM_THIS VIR_FROM_NET + +#define EVENT_DEBUG(fmt, ...) VIR_DEBUG(fmt, __VA_ARGS__) + +#define virNetError(code, ...) \ + virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__) This file is compiled on all platforms, even those that don't support (or have chosen to compile out) netlink support. If you look at netlink.c (which is in the same situation), you'll see that it has appropriate #ifdef WITH_LIBNL lines, and stub functions to replace the working functions when libnl support isn't being compiled in for some reason. You need to do the same thing for this file. Yes, doing so now. + +/* State for a single netlink event handle */ +struct netlinkEventHandle { + int watch; + netlinkEventHandleCallback cb; + void *opaque; + unsigned char macaddr[6]; + int deleted; +}; + +/* State for the main netlink event loop */ +struct netlinkEventLoop { + virMutex lock; + int handeled; + size_t handlesCount; + size_t handlesAlloc; + struct netlinkEventHandle *handles; +}; + +/* Only have one event loop */ +static struct netlinkEventLoop 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 netlinkEventSrvPrivatePtr server = 0; + +/* Function definitions */ +static void +netlinkEventServerLock(netlinkEventSrvPrivatePtr driver) { + virMutexLock(&driver->lock); +} + +static void +netlinkEventServerUnlock(netlinkEventSrvPrivatePtr driver) { + virMutexUnlock(&driver->lock); +} + +static void +netlinkEventCallback(int watch, + int fd ATTRIBUTE_UNUSED, + int events ATTRIBUTE_UNUSED, + void *opaque) { + netlinkEventSrvPrivatePtr srv = opaque; + unsigned char *msg; + struct sockaddr_nl peer; + struct ucred *creds = NULL; + int i, length, handeled; + + length = nl_recv(srv->netlinknh,&peer,&msg,&creds); + + netlinkEventServerLock(srv); + + handeled=0; s/handeled/handled/g + + virMutexLock(&eventLoop.lock); + + VIR_INFO("dispatching to max %d clients, called from event watch %d", + (int)eventLoop.handlesCount, watch); I think most of the VIR_INFOs should be VIR_DEBUG instead. + + for (i = 0 ; i< eventLoop.handlesCount ; i++) { + if (eventLoop.handles[i].deleted) { + continue; + } + + VIR_INFO("dispatching client %d.",i); + + netlinkEventHandleCallback cb = eventLoop.handles[i].cb; + void *cpopaque = eventLoop.handles[i].opaque; + (cb)( msg, length,&peer,&handeled, cpopaque); + } + + virMutexUnlock(&eventLoop.lock); + + if (handeled == 0) { + VIR_INFO("nobody cared."); + } + + 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; + } + } + netlinkEventServerUnlock(srv); +} + +static int +setupNetlinkEventServer(netlinkEventSrvPrivatePtr srv) { + int fd; + + netlinkEventServerLock(srv); + + /* Allocate a new socket and get fd */ + srv->netlinknh = nl_handle_alloc(); + + if (!srv->netlinknh) { + virNetError(errno, + "%s", _("cannot allocate nlhandle for netlinkEvent 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, + netlinkEventCallback, + srv, NULL))< 0) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to add netlink event handle watch")); + + goto exit_cleanup; + } + + srv->netlinkfd = fd; + VIR_INFO("netlink event listener on fd: %i",fd); + + netlinkEventServerUnlock(srv); + return 0; + +exit_cleanup: + nl_close(srv->netlinknh); + nl_handle_destroy(srv->netlinknh); + netlinkEventServerUnlock(srv); + return -1; +} + +/** + * netlinkEventServiceStop: + * + * 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 +netlinkEventServiceStop(void) { + netlinkEventSrvPrivatePtr srv = server; + + VIR_INFO("stopping netlink event service"); + + if (server) { + errno = EINVAL; + return -1; + } I think you mean "if (!server). + + netlinkEventServerLock(srv); + + nl_close(srv->netlinknh); + nl_handle_destroy(srv->netlinknh); + + virEventRemoveHandle(srv->eventwatch); + server=0; + + netlinkEventServerUnlock(srv); + return 0; +} + +/** + * netlinkEventServiceStart: + * + * 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 +netlinkEventServiceStart(void) { + netlinkEventSrvPrivatePtr srv; + + if (server) { + return 0; + } + + VIR_INFO("starting netlink event service"); + + if (VIR_ALLOC(srv)< 0) + goto no_memory; + + if (setupNetlinkEventServer(srv)) { + goto error; + } + + VIR_INFO("netlink event service running"); + + server=srv; + return 0; + + no_memory: + virReportOOMError(); + error: + return -1; +} + +/** + * netlinkEventAddClient: + * + * @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 +netlinkEventAddClient(netlinkEventHandleCallback cb, + void *opaque, + const unsigned char *macaddr) { + int i; + + virMutexLock(&eventLoop.lock); + + VIR_INFO("adding client: %d.",nextWatch); + + /* first try to re-use deleted free slots */ + for (i = 0 ; i< eventLoop.handlesCount ; i++) { + if (eventLoop.handles[i].deleted == 2) { + eventLoop.handles[i].watch = nextWatch; + eventLoop.handles[i].cb = cb; + eventLoop.handles[i].opaque = opaque; + eventLoop.handles[i].deleted = 0; + if (!macaddr) + memcpy(eventLoop.handles[i].macaddr, macaddr,6); You mean "if (macaddr)", not "if (!macaddr)" Also, rather than duplicating the statements that are executed once you find an empty slot, why not have a boolean "found" that's false to begin with, is set to true in the loop if an empty slot is found, and then only do the RESIZE if found is still false (and set "i" accordingly). Then just do the assigment of stuff into the slot once, after you've picked (possibly allocating) a slot. This will prevent future bugs caused by adding something to one of the places you're doing assigment, but not the other (since there will only be one place to do it). Yes, the way it is now it's not good. Also, I don't see any handling for the situation where someone tries to add a client for a macaddress that is already registered. I don't know enough about potential use cases to know whether or not that could be problematic. I don't see the need to register several handlers for the same mac address right now, this would only make sense for other network monitoring purposes, and in any case there would only be one for each interface. More general netlink message handlers can still use their watch id to deregister. + goto cleanup; + } + } + + if (eventLoop.handlesCount == eventLoop.handlesAlloc) { + EVENT_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; + } + } + + eventLoop.handles[eventLoop.handlesCount].watch = nextWatch; + eventLoop.handles[eventLoop.handlesCount].cb = cb; + eventLoop.handles[eventLoop.handlesCount].opaque = opaque; + eventLoop.handles[eventLoop.handlesCount].deleted = 0; + if (!macaddr) + memcpy(eventLoop.handles[i].macaddr, macaddr,6); + + VIR_INFO("added client to loop slot: %d.",(int)eventLoop.handlesCount); + + eventLoop.handlesCount++; + +cleanup: + virMutexUnlock(&eventLoop.lock); + + return nextWatch++; There is a race here (unless the C compiler guarantees that ++ is atomic (including the return value of the _expression_, and I don't think that's the case) - you need to assign nextWatch++ into a temporary variable while the lock is still held, then unlock and return the temp value. Will fix when I redo the above. +} + +/** + * netlinkEventRemoveClient: + * + * @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 +netlinkEventRemoveClient(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_INFO("removed client: %d by index.", + eventLoop.handles[i].watch); + return 0; + } + if (watch == 0&& memcmp(macaddr, eventLoop.handles[i].macaddr, 6)) { + eventLoop.handles[i].deleted = 1; + virMutexUnlock(&eventLoop.lock); + VIR_INFO("removed client: %d by mac.", + eventLoop.handles[i].watch); + return 0; + } + } + virMutexUnlock(&eventLoop.lock); + + VIR_INFO("client not found to remove."); + return -1; +} diff --git a/src/util/netlink-event.h b/src/util/netlink-event.h new file mode 100644 index 0000000..da97395 --- /dev/null +++ b/src/util/netlink-event.h @@ -0,0 +1,63 @@ +/* + * lldpad-event.h: event loop for monitoring netlink messages name change ^^ + * + * 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 + +#include<netlink/netlink.h> + +#include "internal.h" +#include "threads.h" + +typedef struct _netlinkEventSrvPrivate netlinkEventSrvPrivate; +typedef netlinkEventSrvPrivate *netlinkEventSrvPrivatePtr; +struct _netlinkEventSrvPrivate { + virMutex lock; + int eventwatch; + int netlinkfd; + struct nl_handle *netlinknh; +}; + +typedef void (*netlinkEventHandleCallback)( unsigned char *msg, int length, struct sockaddr_nl *peer, int *handled, void *opaque); + +/** + * stopNetlinkEventServer: stop the monitor to receive netlink messages for libvirtd + */ +int netlinkEventServiceStop(void); + +/** + * startNetlinkEventServer: start a monitor to receive netlink messages for libvirtd + */ +int netlinkEventServiceStart(void); + +/** + * netlinkEventAddClient: register a callback for handling of netlink messages + */ +int netlinkEventAddClient(netlinkEventHandleCallback cb, void *opaque, const unsigned char *macaddr); + +/** + * netlinkEventRemoveClient: unregister a callback from a netlink monitor + */ +int netlinkEventRemoveClient(int watch, const unsigned char *macaddr); + +#endif /* NETLINK_EVENT_CONF_H */ diff --git a/src/util/netlink.c b/src/util/netlink.c index 0672184..51bd78a 100644 --- a/src/util/netlink.c +++ b/src/util/netlink.c @@ -131,6 +131,7 @@ err_exit: *respbuflen = 0; } + nl_close(nlhandle); Isn't this an unrelated bugfix? If so, you should submit a separate patch for it. That should'nt have gone in here - The close is supposedly not needed, but it makes the communitation with libvirt more robust it seems. nl_handle_destroy(nlhandle); return rc; } Thank you for the comments ! DirkH -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list |
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list