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@herrendoerfer.name>

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@herrendoerfer.name> --- 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Jan 26, 2012, at 5:57 PM, Daniel P. Berrange wrote:

On Fri, Jan 20, 2012 at 03:56:26PM +0100, D. Herrendoerfer wrote:

...

From: "D. Herrendoerfer" <d.herrendoerfer@herrendoerfer.name>

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@herrendoerfer.name> --- 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 ++++++++

Our current practice is to use a 'vir' prefix on the files, so I'd just use 'virnetlink.[ch]' for this code.

virnetlink.[ch]. ok.

...

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

Don't do this. This is a sign that you need to add some APIs in src/libvirt_private.syms instead.

...

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 @@ -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."); + }

Looking at the code I think it is reasonable to treat this failure as a hard fail. On linux this should always succeed. On non-Linux we should be compiling to a no-op variant.

...

/* Run event loop. */ virNetServerRun(srv);

Probably need to call the Stop method too somewhere....

Yes -

...

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 + * + * Copyright (C) 2011,2012 IBM Corporation. + * Copyright (C) 2011,2012 Dirk Herrendoerfer

s/2011,2012/2011-2012/

...

+#define EVENT_DEBUG(fmt, ...) VIR_DEBUG(fmt, __VA_ARGS__)

Don't do this - just use VIR_DEBUG directly.

Agreed, (all of the above)

...

+/* 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;

I'm not really seeing the point in having two global structs, both with their own lock. I'd say we should just have one struct with all neccessary state in it.

There are three, One is for the event_poll handler, the second for the Service loop, and the third holds the clients data. IMHO it is more complicated to merge them.

...

+ 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);

s/free/VIR_FREE/

...

+int +netlinkEventServiceStop(void) { + netlinkEventSrvPrivatePtr srv = server; + + VIR_INFO("stopping netlink event service"); + + if (server) { + errno = EINVAL; + return -1; + }

IMHO just return 0 here and skip errno.

...

+ + netlinkEventServerLock(srv); + + nl_close(srv->netlinknh); + nl_handle_destroy(srv->netlinknh); + + virEventRemoveHandle(srv->eventwatch); + server=0; + + netlinkEventServerUnlock(srv); + return 0; +} +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);

s/6/VIR_MAC_BUFLEN/

...

+ memcpy(eventLoop.handles[i].macaddr, macaddr,6);

And again.

...

+ + VIR_INFO("added client to loop slot: %d.", (int)eventLoop.handlesCount); + + eventLoop.handlesCount++; + +cleanup: + virMutexUnlock(&eventLoop.lock); + + return nextWatch++; +} +

...

+int +netlinkEventRemoveClient(int watch, const unsigned char *macaddr) {

...

+ if (watch == 0 && memcmp(macaddr, eventLoop.handles[i].macaddr, 6)) {

And here, etc

...

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

Wrong filename

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> + */ + +#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; +};

This shouldn't be made public, and can be merged into the other global state struct.

I agree that it should not be public but I still want to keep them separate. DirkH

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 :|

On 01/20/2012 09:56 AM, D. Herrendoerfer wrote:

From: "D. Herrendoerfer"<d.herrendoerfer@herrendoerfer.name>

Make macvtap setup code register a callback to handle link status changes sent by lldpad.

This is prototype code for reference only.

Signed-off-by: D. Herrendoerfer<d.herrendoerfer@herrendoerfer.name> --- src/util/virnetdevmacvlan.c | 161 ++++++++++++++++++++++++++++++++++++++++++- src/util/virnetdevmacvlan.h | 1 + 2 files changed, 161 insertions(+), 1 deletions(-)

diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 5e55b72..e4a280b 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -46,7 +46,6 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, VIR_NETDEV_MACVLAN_MODE_LAST, "passthrough")

#if WITH_MACVTAP - # include<stdint.h> # include<stdio.h> # include<errno.h> @@ -57,6 +56,8 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, VIR_NETDEV_MACVLAN_MODE_LAST, # include<linux/if.h> # include<linux/if_tun.h>

+# include<c-ctype.h> + /* Older kernels lacked this enum value. */ # if !HAVE_DECL_MACVLAN_MODE_PASSTHRU # define MACVLAN_MODE_PASSTHRU 8 @@ -67,6 +68,7 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, VIR_NETDEV_MACVLAN_MODE_LAST, # include "uuid.h" # include "virfile.h" # include "netlink.h" +# include "netlink-event.h" # include "virnetdev.h"

# define MACVTAP_NAME_PREFIX "macvtap" @@ -75,6 +77,7 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, VIR_NETDEV_MACVLAN_MODE_LAST, # define MACVLAN_NAME_PREFIX "macvlan" # define MACVLAN_NAME_PATTERN "macvlan%d"

+ /** * virNetDevMacVLanCreate: * @@ -445,6 +448,142 @@ static const uint32_t modeMap[VIR_NETDEV_MACVLAN_MODE_LAST] = { [VIR_NETDEV_MACVLAN_MODE_PASSTHRU] = MACVLAN_MODE_PASSTHRU, };

+# define LLDPAD_PID_FILE "/var/run/lldpad.pid" + +static uint32_t +GetLldpadPid(void) {

Instead of this custom function, you should use virPidFileRead, which handles all of these details for you.

+ int fd; + uint32_t pid = 0; + + fd = open(LLDPAD_PID_FILE, O_RDONLY); + if (fd>= 0) { + char buffer[10]; + + if (saferead(fd, buffer, sizeof(buffer))<= sizeof(buffer)) { + unsigned int res; + char *endptr; + + if (virStrToLong_ui(buffer,&endptr, 10,&res) == 0 +&& (*endptr == '\0' || c_isspace(*endptr)) +&& res != 0) { + pid = res; + } else { + virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s", + _("error parsing pid of lldpad")); + } + } + } else { + virReportSystemError(errno, + _("Error opening file %s"), LLDPAD_PID_FILE); + } + + VIR_FORCE_CLOSE(fd); + + return pid; +} + +/* Struct to hold the state and configuration of a 802.1qbg port */ +struct netlinkCallbackData { + char cr_ifname[64]; + virNetDevVPortProfilePtr virtPortProfile; + const unsigned char *macaddress; + const char *linkdev; + const unsigned char *vmuuid; + enum virNetDevVPortProfileOp vmOp; + unsigned int linkState; +}; +typedef struct netlinkCallbackData *netlinkCallbackDataPtr; + +/** + * virNetDevMacVLanVPortProfileCallback: + * + * @msg: The buffer containing the received netlink message + * @length: The length of the received netlink message. + * @peer: The netling sockaddr containing the peer information + * @handeled: Contains information if the message has been replied to yet + * @opaque: Contains vital information regarding the associated vm an interface + * + * This function is called when a netlink message is received. The function + * reads the message and responds if it is pertinent to the running VMs + * network interface. + */ + +static void +virNetDevMacVLanVPortProfileCallback( unsigned char *msg, + int length, + struct sockaddr_nl *peer, + int *handled, + void *opaque) +{

You apparently have tabs turned on in your editor. libvirt coding guidelines require all formatting to be done with spaces (and not trailing whitespace on lines). Please run "make syntax-check" and fix problems it reports prior to submitting any final patch. ("make check" too, but for different reasons)

+ /* ToDo : There is no valid lldpad message yet :( + * + * */ + + struct nlmsghdr *hdr; + void *data; + int i=0; + netlinkCallbackDataPtr calld = opaque; + + VIR_INFO("Netlink message received from nl_sockaddr: %p", peer); + + hdr = (struct nlmsghdr *) msg; + data = nlmsg_data(hdr); + + /* Quickly decide if we want this or not */ + if (hdr->nlmsg_pid != GetLldpadPid()) + return; // we only care for lldpad messages + if (hdr->nlmsg_type != RTM_SETLINK) + return; // we only care for RTM_SETLINK + if (handled != 0) + return; // if it has been handeled - dont handle again

again s/handeled/handled/

+ + /* DEBUG start */ + VIR_INFO("buffer length=%d",length); + VIR_INFO("nlmsg_type = 0x%02x",hdr->nlmsg_type); + VIR_INFO("nlmsg_len = 0x%04x",hdr->nlmsg_len ); + VIR_INFO("nlmsg_pid = %d",hdr->nlmsg_pid ); + VIR_INFO("nlmsg_seq = 0x%08x",hdr->nlmsg_seq ); + VIR_INFO("nlmsg_flags = 0x%04x",hdr->nlmsg_flags ); + + switch (hdr->nlmsg_type) { + case RTM_NEWLINK: + case RTM_DELLINK: + case RTM_SETLINK: + case RTM_GETLINK: + VIR_INFO(" IFINFOMSG\n"); + VIR_INFO(" ifi_family = 0x%02x\n", + ((struct ifinfomsg *)data)->ifi_family); + VIR_INFO(" ifi_type = 0x%x\n", + ((struct ifinfomsg *)data)->ifi_type); + VIR_INFO(" ifi_index = %i\n", + ((struct ifinfomsg *)data)->ifi_index); + VIR_INFO(" ifi_flags = 0x%04x\n", + ((struct ifinfomsg *)data)->ifi_flags); + VIR_INFO(" ifi_change = 0x%04x\n", + ((struct ifinfomsg *)data)->ifi_change); + } + /* DEBUG end */ + + + /* FAKE (try) */ + VIR_INFO("Re-Send associate request:"); + + VIR_INFO(" if: %s",calld->cr_ifname ); + VIR_INFO(" lf: %s",calld->linkdev ); + VIR_INFO(" mac: %04x:%04x:%04x:%04x:%04x:%04x",calld->macaddress[0], + calld->macaddress[1], + calld->macaddress[2], + calld->macaddress[3], + calld->macaddress[4], + calld->macaddress[5] ); + /* ToDo : Send an associate to lldpad + * + * */ + + handled++; + return; +} + /** * virNetDevMacVLanCreateWithVPortProfile: * Create an instance of a macvtap device and open its tap character @@ -589,6 +728,23 @@ create_name: goto disassociate_exit; }

+ if (netlinkEventServiceStart() == 0) { + netlinkCallbackDataPtr calld;

I don't think it's a good idea to call netlinkEventServiceStart here. It will have already been called in main(), and if it failed there, calling it again here (in code that could be executed by multiple threads) could lead to to two threads calling it at the same time, and it is not threadsafe. (I know this is highly unlikely, but... :-) If you want to allow for earlier failure of the service, you should probably make a separate function "netlinkEventServiceIsStarted()".

+ + if (VIR_ALLOC(calld)< 0) { + virReportOOMError(); + goto disassociate_exit; + } + + strncpy(calld->cr_ifname,cr_ifname,64); + calld->virtPortProfile=virtPortProfile; + calld->macaddress=macaddress; + calld->linkdev=linkdev; + calld->vmuuid=vmuuid; + calld->vmOp=vmOp; + + netlinkEventAddClient(virNetDevMacVLanVPortProfileCallback, calld, macaddress); + }

return rc;

@@ -638,6 +794,9 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, if (virNetDevMacVLanDelete(ifname)< 0) ret = -1; } + + netlinkEventRemoveClient(0,macaddr); + return ret; }

diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h index 130ecea..fe54cae 100644 --- a/src/util/virnetdevmacvlan.h +++ b/src/util/virnetdevmacvlan.h @@ -24,6 +24,7 @@ # define __UTIL_MACVTAP_H__

# include "internal.h" +# include "netlink.h" # include "virsocketaddr.h" # include "virnetdevbandwidth.h" # include "virnetdevvportprofile.h"