[libvirt] [PATCH v5 0/2] util: Add netlink event handling code

From: "D. Herrendoerfer" <d.herrendoerfer@herrendoerfer.name> This is the updated netlink event code based upon the updated virnetlink.[ch] files. Also the related code to virnetdevmacvlan is included as well as the related changes to the setup code. D. Herrendoerfer (2): util: Add netlink event handling to virnetlink.c Add de-association handling to macvlan code daemon/libvirtd.c | 8 + src/libvirt_private.syms | 6 + src/qemu/qemu_migration.c | 2 +- src/util/virnetdevmacvlan.c | 315 +++++++++++++++++++++++++++- src/util/virnetdevvportprofile.c | 33 ++- src/util/virnetdevvportprofile.h | 3 +- src/util/virnetlink.c | 438 +++++++++++++++++++++++++++++++++++++- src/util/virnetlink.h | 29 +++ 8 files changed, 815 insertions(+), 19 deletions(-) -- 1.7.7.5

From: "D. Herrendoerfer" <d.herrendoerfer@herrendoerfer.name> This code adds a netlink event interface to libvirt. It is based upon the event_poll code and makes use of it. An event is generated for each netlink message sent to the libvirt pid. Signed-off-by: D. Herrendoerfer <d.herrendoerfer@herrendoerfer.name> --- daemon/libvirtd.c | 8 + src/libvirt_private.syms | 6 + src/util/virnetlink.c | 438 +++++++++++++++++++++++++++++++++++++++++++++- src/util/virnetlink.h | 29 +++ 4 files changed, 476 insertions(+), 5 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index b1b542b..ca8074d 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -47,6 +47,7 @@ #include "conf.h" #include "memory.h" #include "conf.h" +#include "virnetlink.h" #include "virnetserver.h" #include "threads.h" #include "remote.h" @@ -1598,6 +1599,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 +1614,7 @@ int main(int argc, char **argv) { 0, "shutdown", NULL); cleanup: + virNetlinkEventServiceStop(); virNetServerProgramFree(remoteProgram); virNetServerProgramFree(qemuProgram); virNetServerClose(srv); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d6ad36c..008470e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1258,6 +1258,12 @@ virNetDevVPortProfileOpTypeToString; #virnetlink.h virNetlinkCommand; +virNetlinkEventServiceIsRunning; +virNetlinkEventServiceStop; +virNetlinkEventServiceStart; +virNetlinkEventAddClient; +virNetlinkEventRemoveClient; + # virnetmessage.h diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index d03d171..ec4727e 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -35,7 +35,10 @@ #include <sys/types.h> #include "virnetlink.h" +#include "logging.h" #include "memory.h" +#include "threads.h" +#include "virmacaddr.h" #include "virterror_internal.h" #define VIR_FROM_THIS VIR_FROM_NET @@ -46,6 +49,50 @@ #define NETLINK_ACK_TIMEOUT_S 2 +#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; +}; + +typedef struct _virNetlinkEventSrvPrivate virNetlinkEventSrvPrivate; +typedef virNetlinkEventSrvPrivate *virNetlinkEventSrvPrivatePtr; +struct _virNetlinkEventSrvPrivate { + /*Server*/ + virMutex lock; + int eventwatch; + int netlinkfd; + struct nl_handle *netlinknh; + /*Events*/ + int handled; + size_t handlesCount; + size_t handlesAlloc; + struct virNetlinkEventHandle *handles; +}; + +enum virNetlinkDeleteMode { + VIR_NETLINK_HANDLE_VALID, + VIR_NETLINK_HANDLE_DELETED, +}; + +/* 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 = NULL; + +/* Function definitions */ + /** * virNetlinkCommand: * @nlmsg: pointer to netlink message @@ -58,7 +105,6 @@ * Returns 0 on success, -1 on error. In case of error, no response * buffer will be returned. */ -#if defined(__linux__) && defined(HAVE_LIBNL) int virNetlinkCommand(struct nl_msg *nl_msg, unsigned char **respbuf, unsigned int *respbuflen, int nl_pid) @@ -89,7 +135,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, virReportSystemError(errno, "%s", _("cannot connect to netlink socket")); rc = -1; - goto err_exit; + goto error; } nlmsg_set_dst(nl_msg, &nladdr); @@ -101,7 +147,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, virReportSystemError(errno, "%s", _("cannot send to netlink socket")); rc = -1; - goto err_exit; + goto error; } fd = nl_socket_get_fd(nlhandle); @@ -118,7 +164,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, virReportSystemError(ETIMEDOUT, "%s", _("no valid netlink response was received")); rc = -1; - goto err_exit; + goto error; } *respbuflen = nl_recv(nlhandle, &nladdr, respbuf, NULL); @@ -127,7 +173,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, "%s", _("nl_recv failed")); rc = -1; } -err_exit: +error: if (rc == -1) { VIR_FREE(*respbuf); *respbuf = NULL; @@ -138,6 +184,323 @@ err_exit: return rc; } +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; + bool handled = false; + + length = nl_recv(srv->netlinknh, &peer, &msg, &creds); + + if (length == 0) + return; + if (length < 0) { + netlinkError(errno, + "%s", _("nl_recv returned with error")); + return; + } + + virNetlinkEventServerLock(srv); + + VIR_DEBUG("dispatching to max %d clients, called from event watch %d", + (int)srv->handlesCount, watch); + + for (i = 0; i < srv->handlesCount; i++) { + if (srv->handles[i].deleted != VIR_NETLINK_HANDLE_VALID) { + continue; + } + + VIR_DEBUG("dispatching client %d.",i); + + virNetlinkEventHandleCallback cb = srv->handles[i].cb; + void *cpopaque = srv->handles[i].opaque; + (cb)( msg, length, &peer, &handled, cpopaque); + } + + if (!handled) { + VIR_DEBUG("nobody cared."); + } + + VIR_FREE(msg); + + virNetlinkEventServerUnlock(srv); +} + +/** + * 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); + + virMutexDestroy(&srv->lock); + + VIR_FREE(srv); + + return 0; +} + +/** + * virNetlinkEventServiceIsRunning: + * + * returns if the netlink event service is running. + * + * returns 'true' if the service is running, 'false' if stopped. + */ +bool +virNetlinkEventServiceIsRunning(void) { + return (server != NULL); +} + +/** + * 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; + int fd; + int ret = -1; + + if (server) { + return 0; + } + + VIR_INFO("starting netlink event service"); + + if (VIR_ALLOC(srv) < 0) { + virReportOOMError(); + goto error; + } + + /* Init the mutexes */ + if ( virMutexInit(&srv->lock) < 0) + goto error; + + virNetlinkEventServerLock(srv); + + /* Allocate a new socket and get fd */ + srv->netlinknh = nl_handle_alloc(); + + if (!srv->netlinknh) { + netlinkError(errno, + "%s", _("cannot allocate nlhandle for virNetlinkEvent server")); + goto error_locked; + } + + if (nl_connect(srv->netlinknh, NETLINK_ROUTE) < 0) { + netlinkError(errno, + "%s", _("cannot connect to netlink socket")); + goto error_server; + } + + fd = nl_socket_get_fd(srv->netlinknh); + + if (fd < 0) { + netlinkError(errno, + "%s", _("cannot get netlink socket fd")); + goto error_server; + } + + if (nl_socket_set_nonblocking(srv->netlinknh)) { + netlinkError(errno, + "%s", _("cannot set netlink socket nonblocking")); + goto error_server; + } + + if ((srv->eventwatch = virEventAddHandle(fd, + VIR_EVENT_HANDLE_READABLE, + virNetlinkEventCallback, + srv, NULL)) < 0) { + netlinkError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to add netlink event handle watch")); + + goto error_server; + } + + srv->netlinkfd = fd; + VIR_DEBUG("netlink event listener on fd: %i running",fd); + + ret = 0; + server=srv; + +error_server: + if (ret == -1){ + nl_close(srv->netlinknh); + nl_handle_destroy(srv->netlinknh); + } +error_locked: + virNetlinkEventServerUnlock(srv); + if (ret == -1) { + virMutexDestroy(&srv->lock); + VIR_FREE(srv); + } +error: + return ret; +} + +/** + * 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; + virNetlinkEventSrvPrivatePtr srv = server; + + if ( cb == NULL ) + return -1; + + virNetlinkEventServerLock(srv); + + VIR_DEBUG("adding client: %d.",nextWatch); + + r = 0; + /* first try to re-use deleted free slots */ + for (i = 0; i < srv->handlesCount; i++) { + if (srv->handles[i].deleted == VIR_NETLINK_HANDLE_DELETED) { + r = i; + goto addentry; + } + } + /* Resize the eventLoop array if needed */ + if (srv->handlesCount == srv->handlesAlloc) { + VIR_DEBUG("Used %zu handle slots, adding at least %d more", + srv->handlesAlloc, NETLINK_EVENT_ALLOC_EXTENT); + if (VIR_RESIZE_N(srv->handles, srv->handlesAlloc, + srv->handlesCount, NETLINK_EVENT_ALLOC_EXTENT) < 0) { + result = -1; + goto error; + } + } + r = srv->handlesCount++; + +addentry: + srv->handles[r].watch = nextWatch; + srv->handles[r].cb = cb; + srv->handles[r].opaque = opaque; + srv->handles[r].deleted = VIR_NETLINK_HANDLE_VALID; + if (macaddr) + memcpy(srv->handles[r].macaddr, macaddr, VIR_MAC_BUFLEN); + + VIR_DEBUG("added client to loop slot: %d. with macaddr ptr=%p", r, macaddr); + + result = nextWatch++; + +error: + virNetlinkEventServerUnlock(srv); + + 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; + int ret = -1; + virNetlinkEventSrvPrivatePtr srv = server; + + VIR_DEBUG("removing client watch=%d, mac=%p.", + watch, macaddr); + + if (watch <= 0 && !macaddr) { + VIR_WARN("Ignoring invalid netlink client id: %d", watch); + return -1; + } + + virNetlinkEventServerLock(srv); + + for (i = 0; i < srv->handlesCount; i++) { + if (srv->handles[i].deleted != VIR_NETLINK_HANDLE_VALID) + continue; + + if (watch && srv->handles[i].watch == watch) { + VIR_FREE(srv->handles[i].opaque); + srv->handles[i].deleted = VIR_NETLINK_HANDLE_DELETED; + VIR_DEBUG("removed client: %d by index.", + srv->handles[i].watch); + ret = 0; + goto error; + } + if (!watch && memcmp(macaddr, srv->handles[i].macaddr, VIR_MAC_BUFLEN) == 0) { + VIR_FREE(srv->handles[i].opaque); + srv->handles[i].deleted = VIR_NETLINK_HANDLE_DELETED; + VIR_DEBUG("removed client: %d by mac.", + srv->handles[i].watch); + ret = 0; + goto error; + } + } + VIR_DEBUG("client not found to remove."); + +error: + virNetlinkEventServerUnlock(srv); + return ret; +} + #else int virNetlinkCommand(struct nl_msg *nl_msg ATTRIBUTE_UNUSED, @@ -154,4 +517,69 @@ int virNetlinkCommand(struct nl_msg *nl_msg ATTRIBUTE_UNUSED, return -1; } +/** + * stopNetlinkEventServer: stop the monitor to receive netlink messages for libvirtd + */ +int virNetlinkEventServiceStop(void) { + netlinkError(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) { +# if defined(__linux__) && !defined(HAVE_LIBNL) + netlinkError(VIR_ERR_INTERNAL_ERROR, + "%s", + _("virNetlinkEventServiceStart is not supported since libnl was not available")); +# endif + return 0; +} + +/** + * virNetlinkEventServiceIsRunning: returns if the netlink event service is running. + */ +int virNetlinkEventServiceIsRunning(void) { +# if defined(__linux__) && !defined(HAVE_LIBNL) + netlinkError(VIR_ERR_INTERNAL_ERROR, + "%s", + _("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) { + netlinkError(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) { + netlinkError(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__ */ diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index a70abb6..1365afa 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -21,6 +21,7 @@ # define __VIR_NETLINK_H__ # include "config.h" +# include "internal.h" # if defined(__linux__) && defined(HAVE_LIBNL) @@ -29,6 +30,7 @@ # else struct nl_msg; +struct sockaddr_nl; # endif /* __linux__ */ @@ -36,4 +38,31 @@ int virNetlinkCommand(struct nl_msg *nl_msg, unsigned char **respbuf, unsigned int *respbuflen, int nl_pid); +typedef void (*virNetlinkEventHandleCallback)( unsigned char *msg, int length, struct sockaddr_nl *peer, bool *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. + */ +bool 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 /* __VIR_NETLINK_H__ */ -- 1.7.7.5

On 02/22/2012 08:17 AM, D. Herrendoerfer wrote:
From: "D. Herrendoerfer" <d.herrendoerfer@herrendoerfer.name>
This code adds a netlink event interface to libvirt. It is based upon the event_poll code and makes use of it. An event is generated for each netlink message sent to the libvirt pid.
Signed-off-by: D. Herrendoerfer <d.herrendoerfer@herrendoerfer.name> --- daemon/libvirtd.c | 8 + src/libvirt_private.syms | 6 + src/util/virnetlink.c | 438 +++++++++++++++++++++++++++++++++++++++++++++- src/util/virnetlink.h | 29 +++ 4 files changed, 476 insertions(+), 5 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index b1b542b..ca8074d 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -47,6 +47,7 @@ #include "conf.h" #include "memory.h" #include "conf.h" +#include "virnetlink.h" #include "virnetserver.h" #include "threads.h" #include "remote.h" @@ -1598,6 +1599,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 +1614,7 @@ int main(int argc, char **argv) { 0, "shutdown", NULL);
cleanup: + virNetlinkEventServiceStop(); virNetServerProgramFree(remoteProgram); virNetServerProgramFree(qemuProgram); virNetServerClose(srv); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d6ad36c..008470e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1258,6 +1258,12 @@ virNetDevVPortProfileOpTypeToString;
#virnetlink.h virNetlinkCommand; +virNetlinkEventServiceIsRunning; +virNetlinkEventServiceStop; +virNetlinkEventServiceStart; +virNetlinkEventAddClient; +virNetlinkEventRemoveClient; +
# virnetmessage.h diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index d03d171..ec4727e 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -35,7 +35,10 @@ #include <sys/types.h>
#include "virnetlink.h" +#include "logging.h" #include "memory.h" +#include "threads.h" +#include "virmacaddr.h" #include "virterror_internal.h"
#define VIR_FROM_THIS VIR_FROM_NET @@ -46,6 +49,50 @@
#define NETLINK_ACK_TIMEOUT_S 2
+#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; +}; + +typedef struct _virNetlinkEventSrvPrivate virNetlinkEventSrvPrivate; +typedef virNetlinkEventSrvPrivate *virNetlinkEventSrvPrivatePtr; +struct _virNetlinkEventSrvPrivate { + /*Server*/ + virMutex lock; + int eventwatch; + int netlinkfd; + struct nl_handle *netlinknh; + /*Events*/ + int handled; + size_t handlesCount; + size_t handlesAlloc; + struct virNetlinkEventHandle *handles; +}; + +enum virNetlinkDeleteMode { + VIR_NETLINK_HANDLE_VALID, + VIR_NETLINK_HANDLE_DELETED, +}; + +/* Only have one event loop */ +//static struct virNetlinkEventLoop eventLoop;
You commented this out but forgot to remove it.
+ +/* 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 = NULL; + +/* Function definitions */ + /** * virNetlinkCommand: * @nlmsg: pointer to netlink message @@ -58,7 +105,6 @@ * Returns 0 on success, -1 on error. In case of error, no response * buffer will be returned. */ -#if defined(__linux__) && defined(HAVE_LIBNL) int virNetlinkCommand(struct nl_msg *nl_msg, unsigned char **respbuf, unsigned int *respbuflen, int nl_pid) @@ -89,7 +135,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, virReportSystemError(errno, "%s", _("cannot connect to netlink socket")); rc = -1; - goto err_exit; + goto error; }
nlmsg_set_dst(nl_msg, &nladdr); @@ -101,7 +147,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, virReportSystemError(errno, "%s", _("cannot send to netlink socket")); rc = -1; - goto err_exit; + goto error; }
fd = nl_socket_get_fd(nlhandle); @@ -118,7 +164,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, virReportSystemError(ETIMEDOUT, "%s", _("no valid netlink response was received")); rc = -1; - goto err_exit; + goto error; }
*respbuflen = nl_recv(nlhandle, &nladdr, respbuf, NULL); @@ -127,7 +173,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, "%s", _("nl_recv failed")); rc = -1; } -err_exit: +error: if (rc == -1) { VIR_FREE(*respbuf); *respbuf = NULL; @@ -138,6 +184,323 @@ err_exit: return rc; }
+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; + bool handled = false; + + length = nl_recv(srv->netlinknh, &peer, &msg, &creds); + + if (length == 0) + return; + if (length < 0) { + netlinkError(errno, + "%s", _("nl_recv returned with error")); + return; + } + + virNetlinkEventServerLock(srv); + + VIR_DEBUG("dispatching to max %d clients, called from event watch %d", + (int)srv->handlesCount, watch); + + for (i = 0; i < srv->handlesCount; i++) { + if (srv->handles[i].deleted != VIR_NETLINK_HANDLE_VALID) { + continue; + } + + VIR_DEBUG("dispatching client %d.",i); + + virNetlinkEventHandleCallback cb = srv->handles[i].cb; + void *cpopaque = srv->handles[i].opaque; + (cb)( msg, length, &peer, &handled, cpopaque); + } + + if (!handled) { + VIR_DEBUG("nobody cared.");
We might want to say something a little less informal :-)
+ } + + VIR_FREE(msg); + + virNetlinkEventServerUnlock(srv); +} + +/** + * 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); + + virMutexDestroy(&srv->lock); + + VIR_FREE(srv); + + return 0; +} + +/** + * virNetlinkEventServiceIsRunning: + * + * returns if the netlink event service is running. + * + * returns 'true' if the service is running, 'false' if stopped. + */ +bool +virNetlinkEventServiceIsRunning(void) { + return (server != NULL); +} + +/** + * 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; + int fd; + int ret = -1; + + if (server) { + return 0; + } + + VIR_INFO("starting netlink event service"); + + if (VIR_ALLOC(srv) < 0) { + virReportOOMError(); + goto error; + } + + /* Init the mutexes */
There's now only one mutex, and the name of the function makes the comment a bit redundant.
+ if ( virMutexInit(&srv->lock) < 0) + goto error; + + virNetlinkEventServerLock(srv); + + /* Allocate a new socket and get fd */ + srv->netlinknh = nl_handle_alloc(); + + if (!srv->netlinknh) { + netlinkError(errno, + "%s", _("cannot allocate nlhandle for virNetlinkEvent server")); + goto error_locked; + } + + if (nl_connect(srv->netlinknh, NETLINK_ROUTE) < 0) { + netlinkError(errno, + "%s", _("cannot connect to netlink socket")); + goto error_server; + } + + fd = nl_socket_get_fd(srv->netlinknh); + + if (fd < 0) { + netlinkError(errno, + "%s", _("cannot get netlink socket fd")); + goto error_server; + } + + if (nl_socket_set_nonblocking(srv->netlinknh)) { + netlinkError(errno, + "%s", _("cannot set netlink socket nonblocking")); + goto error_server; + } + + if ((srv->eventwatch = virEventAddHandle(fd, + VIR_EVENT_HANDLE_READABLE, + virNetlinkEventCallback, + srv, NULL)) < 0) { + netlinkError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to add netlink event handle watch")); + + goto error_server; + } + + srv->netlinkfd = fd; + VIR_DEBUG("netlink event listener on fd: %i running",fd); + + ret = 0; + server=srv; + +error_server: + if (ret == -1){ + nl_close(srv->netlinknh); + nl_handle_destroy(srv->netlinknh); + } +error_locked: + virNetlinkEventServerUnlock(srv); + if (ret == -1) { + virMutexDestroy(&srv->lock); + VIR_FREE(srv); + } +error: + return ret; +} + +/** + * 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; + virNetlinkEventSrvPrivatePtr srv = server; + + if ( cb == NULL ) + return -1; + + virNetlinkEventServerLock(srv); + + VIR_DEBUG("adding client: %d.",nextWatch); + + r = 0; + /* first try to re-use deleted free slots */ + for (i = 0; i < srv->handlesCount; i++) { + if (srv->handles[i].deleted == VIR_NETLINK_HANDLE_DELETED) { + r = i; + goto addentry; + } + } + /* Resize the eventLoop array if needed */ + if (srv->handlesCount == srv->handlesAlloc) { + VIR_DEBUG("Used %zu handle slots, adding at least %d more", + srv->handlesAlloc, NETLINK_EVENT_ALLOC_EXTENT); + if (VIR_RESIZE_N(srv->handles, srv->handlesAlloc, + srv->handlesCount, NETLINK_EVENT_ALLOC_EXTENT) < 0) { + result = -1; + goto error; + } + } + r = srv->handlesCount++; + +addentry: + srv->handles[r].watch = nextWatch; + srv->handles[r].cb = cb; + srv->handles[r].opaque = opaque; + srv->handles[r].deleted = VIR_NETLINK_HANDLE_VALID; + if (macaddr) + memcpy(srv->handles[r].macaddr, macaddr, VIR_MAC_BUFLEN); + + VIR_DEBUG("added client to loop slot: %d. with macaddr ptr=%p", r, macaddr); + + result = nextWatch++; + +error: + virNetlinkEventServerUnlock(srv); + + 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; + int ret = -1; + virNetlinkEventSrvPrivatePtr srv = server; + + VIR_DEBUG("removing client watch=%d, mac=%p.", + watch, macaddr); + + if (watch <= 0 && !macaddr) { + VIR_WARN("Ignoring invalid netlink client id: %d", watch); + return -1; + } + + virNetlinkEventServerLock(srv); + + for (i = 0; i < srv->handlesCount; i++) { + if (srv->handles[i].deleted != VIR_NETLINK_HANDLE_VALID) + continue; + + if (watch && srv->handles[i].watch == watch) { + VIR_FREE(srv->handles[i].opaque); + srv->handles[i].deleted = VIR_NETLINK_HANDLE_DELETED; + VIR_DEBUG("removed client: %d by index.", + srv->handles[i].watch); + ret = 0; + goto error; + } + if (!watch && memcmp(macaddr, srv->handles[i].macaddr, VIR_MAC_BUFLEN) == 0) { + VIR_FREE(srv->handles[i].opaque); + srv->handles[i].deleted = VIR_NETLINK_HANDLE_DELETED; + VIR_DEBUG("removed client: %d by mac.", + srv->handles[i].watch); + ret = 0; + goto error; + } + } + VIR_DEBUG("client not found to remove."); + +error: + virNetlinkEventServerUnlock(srv); + return ret; +} + #else
int virNetlinkCommand(struct nl_msg *nl_msg ATTRIBUTE_UNUSED, @@ -154,4 +517,69 @@ int virNetlinkCommand(struct nl_msg *nl_msg ATTRIBUTE_UNUSED, return -1; }
+/** + * stopNetlinkEventServer: stop the monitor to receive netlink messages for libvirtd + */ +int virNetlinkEventServiceStop(void) { + netlinkError(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) { +# if defined(__linux__) && !defined(HAVE_LIBNL) + netlinkError(VIR_ERR_INTERNAL_ERROR, + "%s", + _("virNetlinkEventServiceStart is not supported since libnl was not available")); +# endif + return 0; +} + +/** + * virNetlinkEventServiceIsRunning: returns if the netlink event service is running. + */ +int virNetlinkEventServiceIsRunning(void) { +# if defined(__linux__) && !defined(HAVE_LIBNL) + netlinkError(VIR_ERR_INTERNAL_ERROR, + "%s", + _("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) { + netlinkError(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) { + netlinkError(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__ */ diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index a70abb6..1365afa 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -21,6 +21,7 @@ # define __VIR_NETLINK_H__
# include "config.h" +# include "internal.h"
# if defined(__linux__) && defined(HAVE_LIBNL)
@@ -29,6 +30,7 @@ # else
struct nl_msg; +struct sockaddr_nl;
# endif /* __linux__ */
@@ -36,4 +38,31 @@ int virNetlinkCommand(struct nl_msg *nl_msg, unsigned char **respbuf, unsigned int *respbuflen, int nl_pid);
+typedef void (*virNetlinkEventHandleCallback)( unsigned char *msg, int length, struct sockaddr_nl *peer, bool *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. + */ +bool 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 /* __VIR_NETLINK_H__ */
This looks very good - just the three very minor nits. ACK to this; I'll fix the nits and push it pending review of you answer to my question in PATCH 2/2

From: "D. Herrendoerfer" <d.herrendoerfer@herrendoerfer.name> Add de-association handling for 802.1qbg (vepa) via lldpad netlink messages. Also adds the possibility to perform an association request without waiting for a confirmation. Signed-off-by: D. Herrendoerfer <d.herrendoerfer@herrendoerfer.name> --- src/qemu/qemu_migration.c | 2 +- src/util/virnetdevmacvlan.c | 315 +++++++++++++++++++++++++++++++++++++- src/util/virnetdevvportprofile.c | 33 +++-- src/util/virnetdevvportprofile.h | 3 +- 4 files changed, 339 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 12cfbde..31428f8 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2567,7 +2567,7 @@ qemuMigrationVPAssociatePortProfiles(virDomainDefPtr def) { net->mac, virDomainNetGetActualDirectDev(net), def->uuid, - VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH) < 0) + VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH, false) < 0) goto err_exit; } last_good_net = i; diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 25d0846..b3e3325 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 @@ -68,6 +69,8 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, VIR_NETDEV_MACVLAN_MODE_LAST, # include "virfile.h" # include "virnetlink.h" # include "virnetdev.h" +# include "virpidfile.h" + # define MACVTAP_NAME_PREFIX "macvtap" # define MACVTAP_NAME_PATTERN "macvtap%d" @@ -75,6 +78,7 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, VIR_NETDEV_MACVLAN_MODE_LAST, # define MACVLAN_NAME_PREFIX "macvlan" # define MACVLAN_NAME_PATTERN "macvlan%d" + /** * virNetDevMacVLanCreate: * @@ -445,6 +449,293 @@ static const uint32_t modeMap[VIR_NETDEV_MACVLAN_MODE_LAST] = { [VIR_NETDEV_MACVLAN_MODE_PASSTHRU] = MACVLAN_MODE_PASSTHRU, }; +/* Struct to hold the state and configuration of a 802.1qbg port */ +struct virNetlinkCallbackData { + 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 virNetlinkCallbackData *virNetlinkCallbackDataPtr; + +#define INSTANCE_STRLEN 36 + +static int instance2str(const unsigned char *p, char *dst, size_t size) +{ + if (dst && size > INSTANCE_STRLEN) { + snprintf(dst, size, "%02x%02x%02x%02x-%02x%02x-%02x%02x-" + "%02x%02x-%02x%02x%02x%02x%02x%02x", + p[0], p[1], p[2], p[3], + p[4], p[5], p[6], p[7], + p[8], p[9], p[10], p[11], p[12], p[13], p[14], p[15]); + return 0; + } + return -1; +} + +# define LLDPAD_PID_FILE "/var/run/lldpad.pid" +# define VIRIP_PID_FILE "/var/run/virip.pid" + +/** + * 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, + bool *handled, + void *opaque) +{ + struct nla_policy ifla_vf_policy[IFLA_VF_MAX + 1] = { + [IFLA_VF_MAC] = {.minlen = sizeof(struct ifla_vf_mac), + .maxlen = sizeof(struct ifla_vf_mac)}, + [IFLA_VF_VLAN] = {.minlen = sizeof(struct ifla_vf_vlan), + .maxlen = sizeof(struct ifla_vf_vlan)}, + }; + + struct nla_policy ifla_port_policy[IFLA_PORT_MAX + 1] = { + [IFLA_PORT_RESPONSE] = {.type = NLA_U16}, + }; + + struct nlattr *tb[IFLA_MAX + 1], + *tb3[IFLA_PORT_MAX + 1], + *tb_vfinfo[IFLA_VF_MAX + 1], *tb_vfinfo_list; + + struct ifinfomsg ifinfo; + struct nlmsghdr *hdr; + void *data; + int rem; + char *ifname; + bool indicate = false; + virNetlinkCallbackDataPtr calld = opaque; + pid_t lldpad_pid = 0; + pid_t virip_pid = 0; + + hdr = (struct nlmsghdr *) msg; + data = nlmsg_data(hdr); + + /* Quickly decide if we want this or not */ + + if (virPidFileReadPath(LLDPAD_PID_FILE, &lldpad_pid) < 0) + return; + + ignore_value(virPidFileReadPath(VIRIP_PID_FILE, &virip_pid)); + + if (hdr->nlmsg_pid != lldpad_pid && hdr->nlmsg_pid != virip_pid) + return; // we only care for lldpad and virip messages + if (hdr->nlmsg_type != RTM_SETLINK) + return; // we only care for RTM_SETLINK + if (*handled) + return; // if it has been handeled - dont handle again + + /* DEBUG start */ + VIR_INFO("netlink message nl_sockaddr: %p len: %d", peer, length); + VIR_DEBUG("nlmsg_type = 0x%02x",hdr->nlmsg_type); + VIR_DEBUG("nlmsg_len = 0x%04x",hdr->nlmsg_len ); + VIR_DEBUG("nlmsg_pid = %d",hdr->nlmsg_pid ); + VIR_DEBUG("nlmsg_seq = 0x%08x",hdr->nlmsg_seq ); + VIR_DEBUG("nlmsg_flags = 0x%04x",hdr->nlmsg_flags ); + + VIR_DEBUG("lldpad pid = %d",lldpad_pid); + + switch (hdr->nlmsg_type) { + case RTM_NEWLINK: + case RTM_DELLINK: + case RTM_SETLINK: + case RTM_GETLINK: + VIR_DEBUG(" IFINFOMSG\n"); + VIR_DEBUG(" ifi_family = 0x%02x\n", + ((struct ifinfomsg *)data)->ifi_family); + VIR_DEBUG(" ifi_type = 0x%x\n", + ((struct ifinfomsg *)data)->ifi_type); + VIR_DEBUG(" ifi_index = %i\n", + ((struct ifinfomsg *)data)->ifi_index); + VIR_DEBUG(" ifi_flags = 0x%04x\n", + ((struct ifinfomsg *)data)->ifi_flags); + VIR_DEBUG(" ifi_change = 0x%04x\n", + ((struct ifinfomsg *)data)->ifi_change); + } + /* DEBUG end */ + + /* Parse netlink message assume a setlink with vfports */ + memcpy(&ifinfo, NLMSG_DATA(hdr), sizeof ifinfo); + VIR_DEBUG("family:%#x type:%#x index:%d flags:%#x change:%#x", + ifinfo.ifi_family, ifinfo.ifi_type, ifinfo.ifi_index, + ifinfo.ifi_flags, ifinfo.ifi_change); + if (nlmsg_parse(hdr, sizeof ifinfo, + (struct nlattr **)&tb, IFLA_MAX, NULL)) { + VIR_DEBUG("error parsing request..."); + return; + } + + if (tb[IFLA_VFINFO_LIST]) { + VIR_DEBUG("FOUND IFLA_VFINFO_LIST!"); + + nla_for_each_nested(tb_vfinfo_list, tb[IFLA_VFINFO_LIST], rem) { + if (nla_type(tb_vfinfo_list) != IFLA_VF_INFO) { + VIR_DEBUG("nested parsing of" + "IFLA_VFINFO_LIST failed."); + return; + } + if (nla_parse_nested(tb_vfinfo, IFLA_VF_MAX, + tb_vfinfo_list, ifla_vf_policy)) { + VIR_DEBUG("nested parsing of " + "IFLA_VF_INFO failed."); + return; + } + } + + if (tb_vfinfo[IFLA_VF_MAC]) { + struct ifla_vf_mac *mac = + RTA_DATA(tb_vfinfo[IFLA_VF_MAC]); + unsigned char *m = mac->mac; + + VIR_DEBUG("IFLA_VF_MAC=%2x:%2x:%2x:%2x:%2x:%2x", + m[0], m[1], m[2], m[3], m[4], m[5]); + + if (memcmp(calld->macaddress, m, VIR_MAC_BUFLEN)) + { + /* Repeat the same check for a broadcast mac */ + unsigned char broadcastmac[VIR_MAC_BUFLEN]; + int i; + + for (i=0;i<VIR_MAC_BUFLEN;i++) + broadcastmac[i]=0xFF; + + if (memcmp(calld->macaddress, broadcastmac, VIR_MAC_BUFLEN)) { + VIR_DEBUG("MAC address match failed."); + return; + } + } + } + + if (tb_vfinfo[IFLA_VF_VLAN]) { + struct ifla_vf_vlan *vlan = + RTA_DATA(tb_vfinfo[IFLA_VF_VLAN]); + + VIR_DEBUG("IFLA_VF_VLAN=%d", vlan->vlan); + } + } + + if (tb[IFLA_IFNAME]) { + ifname = (char *)RTA_DATA(tb[IFLA_IFNAME]); + VIR_DEBUG("IFLA_IFNAME=%s\n", ifname); + } + + if (tb[IFLA_OPERSTATE]) { + rem = *(unsigned short *)RTA_DATA(tb[IFLA_OPERSTATE]); + VIR_DEBUG("IFLA_OPERSTATE=%d\n", rem); + } + + if (tb[IFLA_VF_PORTS]) { + struct nlattr *tb_vf_ports; + + VIR_DEBUG("found IFLA_VF_PORTS\n"); + nla_for_each_nested(tb_vf_ports, tb[IFLA_VF_PORTS], rem) { + + VIR_DEBUG("iterating\n"); + if (nla_type(tb_vf_ports) != IFLA_VF_PORT) { + VIR_DEBUG("not a IFLA_VF_PORT. skipping\n"); + continue; + } + if (nla_parse_nested(tb3, IFLA_PORT_MAX, tb_vf_ports, + ifla_port_policy)) { + VIR_DEBUG("nested parsing on level 2" + " failed."); + } + if (tb3[IFLA_PORT_VF]) { + VIR_DEBUG("IFLA_PORT_VF=%d", + *(uint32_t + *) (RTA_DATA(tb3[IFLA_PORT_VF]))); + } + if (tb3[IFLA_PORT_PROFILE]) { + VIR_DEBUG("IFLA_PORT_PROFILE=%s", (char *) + RTA_DATA(tb3[IFLA_PORT_PROFILE])); + } + + if (tb3[IFLA_PORT_VSI_TYPE]) { + struct ifla_port_vsi *pvsi; + int tid = 0; + + pvsi = (struct ifla_port_vsi *) + RTA_DATA(tb3[IFLA_PORT_VSI_TYPE]); + tid = pvsi->vsi_type_id[2] << 16 | + pvsi->vsi_type_id[1] << 8 | + pvsi->vsi_type_id[0]; + + VIR_DEBUG("mgr_id: %d", pvsi->vsi_mgr_id); + VIR_DEBUG("type_id: %d", tid); + VIR_DEBUG("type_version: %d", + pvsi->vsi_type_version); + } + + if (tb3[IFLA_PORT_INSTANCE_UUID]) { + char instance[INSTANCE_STRLEN + 2]; + unsigned char *uuid; + + uuid = (unsigned char *) + RTA_DATA(tb3[IFLA_PORT_INSTANCE_UUID]); + instance2str(uuid, instance, sizeof(instance)); + VIR_DEBUG("IFLA_PORT_INSTANCE_UUID=%s\n", + instance); + } + + if (tb3[IFLA_PORT_REQUEST]) { + uint8_t req = *(uint8_t *) RTA_DATA(tb3[IFLA_PORT_REQUEST]); + VIR_DEBUG("IFLA_PORT_REQUEST=%d", req); + + if ( req == PORT_REQUEST_DISASSOCIATE ) { + VIR_DEBUG("Set dissaccociated."); + indicate=true; + } + } + + if (tb3[IFLA_PORT_RESPONSE]) { + VIR_DEBUG("IFLA_PORT_RESPONSE=%d\n", *(uint16_t *) + RTA_DATA(tb3[IFLA_PORT_RESPONSE])); + } + } + } + + if (!indicate) { + return; + } + + VIR_INFO("Re-send 802.1qbg associate request:"); + VIR_INFO(" if: %s",calld->cr_ifname ); + VIR_INFO(" lf: %s",calld->linkdev ); + VIR_INFO(" mac: %02x:%02x:%02x:%02x:%02x:%02x",calld->macaddress[0], + calld->macaddress[1], + calld->macaddress[2], + calld->macaddress[3], + calld->macaddress[4], + calld->macaddress[5] ); + + ignore_value(virNetDevVPortProfileAssociate(calld->cr_ifname, + calld->virtPortProfile, + calld->macaddress, + calld->linkdev, + calld->vmuuid, calld->vmOp, true)); + + *handled = true; + return; +} + /** * virNetDevMacVLanCreateWithVPortProfile: * Create an instance of a macvtap device and open its tap character @@ -547,7 +838,7 @@ create_name: virtPortProfile, macaddress, linkdev, - vmuuid, vmOp) < 0) { + vmuuid, vmOp, false) < 0) { rc = -1; goto link_del_exit; } @@ -589,6 +880,23 @@ create_name: goto disassociate_exit; } + if (virNetlinkEventServiceIsRunning()) { + virNetlinkCallbackDataPtr calld; + + 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; + + virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback, calld, macaddress); + } return rc; @@ -638,6 +946,9 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, if (virNetDevMacVLanDelete(ifname) < 0) ret = -1; } + + virNetlinkEventRemoveClient(0,macaddr); + return ret; } diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index faadc3a..554f128 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -650,7 +650,8 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex, const unsigned char *instanceId, const unsigned char *hostUUID, int32_t vf, - uint8_t op) + uint8_t op, + bool setlink_only) { int rc; unsigned char *recvbuf = NULL; @@ -675,6 +676,9 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex, return rc; } + if (setlink_only) /*for re-associations on existing links*/ + return 0; + while (--repeats >= 0) { rc = virNetDevVPortProfileLinkDump(NULL, ifindex, nltarget_kernel, tb, &recvbuf, virNetDevVPortProfileGetLldpadPid); @@ -751,7 +755,8 @@ static int virNetDevVPortProfileOp8021Qbg(const char *ifname, const unsigned char *macaddr, const virNetDevVPortProfilePtr virtPort, - enum virNetDevVPortProfileLinkOp virtPortOp) + enum virNetDevVPortProfileLinkOp virtPortOp, + bool setlink_only) { int rc = 0; int op = PORT_REQUEST_ASSOCIATE; @@ -804,7 +809,8 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname, virtPort->u.virtPort8021Qbg.instanceID, NULL, vf, - op); + op, + setlink_only); err_exit: @@ -892,7 +898,8 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname, (virtPortOp == VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE_RR) ? PORT_REQUEST_PREASSOCIATE_RR - : PORT_REQUEST_ASSOCIATE); + : PORT_REQUEST_ASSOCIATE, + false); if (rc == -2) /* Association timed out, disassociate */ virNetDevVPortProfileOpCommon(NULL, ifindex, @@ -904,7 +911,8 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname, NULL, NULL, vf, - PORT_REQUEST_DISASSOCIATE); + PORT_REQUEST_DISASSOCIATE, + false); break; case VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE: @@ -917,7 +925,8 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname, NULL, NULL, vf, - PORT_REQUEST_DISASSOCIATE); + PORT_REQUEST_DISASSOCIATE, + false); break; default: @@ -938,6 +947,7 @@ err_exit: * @virtPort: pointer to the object holding port profile parameters * @vmuuid : the UUID of the virtual machine * @vmOp : The VM operation (i.e., create, no-op) + * @setlink_only : Only set the link - dont wait for the link to come up * * Associate a port on a swtich with a profile. This function * may notify a kernel driver or an external daemon to run @@ -954,7 +964,8 @@ virNetDevVPortProfileAssociate(const char *macvtap_ifname, const unsigned char *macvtap_macaddr, const char *linkdev, const unsigned char *vmuuid, - enum virNetDevVPortProfileOp vmOp) + enum virNetDevVPortProfileOp vmOp, + bool setlink_only) { int rc = 0; @@ -976,7 +987,8 @@ virNetDevVPortProfileAssociate(const char *macvtap_ifname, virtPort, (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) ? VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE - : VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE); + : VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE, + setlink_only); break; case VIR_NETDEV_VPORT_PROFILE_8021QBH: @@ -1033,7 +1045,7 @@ virNetDevVPortProfileDisassociate(const char *macvtap_ifname, case VIR_NETDEV_VPORT_PROFILE_8021QBG: rc = virNetDevVPortProfileOp8021Qbg(macvtap_ifname, macvtap_macaddr, virtPort, - VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE); + VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE, false); break; case VIR_NETDEV_VPORT_PROFILE_8021QBH: @@ -1056,7 +1068,8 @@ int virNetDevVPortProfileAssociate(const char *macvtap_ifname ATTRIBUTE_UNUSED, const unsigned char *macvtap_macaddr ATTRIBUTE_UNUSED, const char *linkdev ATTRIBUTE_UNUSED, const unsigned char *vmuuid ATTRIBUTE_UNUSED, - enum virNetDevVPortProfileOp vmOp ATTRIBUTE_UNUSED) + enum virNetDevVPortProfileOp vmOp ATTRIBUTE_UNUSED, + bool setlink_only) { virReportSystemError(ENOSYS, "%s", _("Virtual port profile association not supported on this platform")); diff --git a/src/util/virnetdevvportprofile.h b/src/util/virnetdevvportprofile.h index 7a8d81f..fe80086 100644 --- a/src/util/virnetdevvportprofile.h +++ b/src/util/virnetdevvportprofile.h @@ -81,7 +81,8 @@ int virNetDevVPortProfileAssociate(const char *ifname, const unsigned char *macaddr, const char *linkdev, const unsigned char *vmuuid, - enum virNetDevVPortProfileOp vmOp) + enum virNetDevVPortProfileOp vmOp, + bool setlink_only) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; -- 1.7.7.5

On 02/22/2012 08:17 AM, D. Herrendoerfer wrote:
From: "D. Herrendoerfer" <d.herrendoerfer@herrendoerfer.name>
Add de-association handling for 802.1qbg (vepa) via lldpad netlink messages. Also adds the possibility to perform an association request without waiting for a confirmation.
The main issue I see with this patch is with whitespace, but that can easily be fixed prior to pushing it, so ACK. Is the message format used by this patch, the absolute final version? (i.e. can we safely push it an know that it will be correct?)
Signed-off-by: D. Herrendoerfer <d.herrendoerfer@herrendoerfer.name> --- src/qemu/qemu_migration.c | 2 +- src/util/virnetdevmacvlan.c | 315 +++++++++++++++++++++++++++++++++++++- src/util/virnetdevvportprofile.c | 33 +++-- src/util/virnetdevvportprofile.h | 3 +- 4 files changed, 339 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 12cfbde..31428f8 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2567,7 +2567,7 @@ qemuMigrationVPAssociatePortProfiles(virDomainDefPtr def) { net->mac, virDomainNetGetActualDirectDev(net), def->uuid, - VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH) < 0) + VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH, false) < 0) goto err_exit; } last_good_net = i; diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 25d0846..b3e3325 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 -
spurious whitespace change.
# 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 @@ -68,6 +69,8 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, VIR_NETDEV_MACVLAN_MODE_LAST, # include "virfile.h" # include "virnetlink.h" # include "virnetdev.h" +# include "virpidfile.h" +
# define MACVTAP_NAME_PREFIX "macvtap" # define MACVTAP_NAME_PATTERN "macvtap%d" @@ -75,6 +78,7 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, VIR_NETDEV_MACVLAN_MODE_LAST, # define MACVLAN_NAME_PREFIX "macvlan" # define MACVLAN_NAME_PATTERN "macvlan%d"
+
Another spurious whitespace change.
/** * virNetDevMacVLanCreate: * @@ -445,6 +449,293 @@ static const uint32_t modeMap[VIR_NETDEV_MACVLAN_MODE_LAST] = { [VIR_NETDEV_MACVLAN_MODE_PASSTHRU] = MACVLAN_MODE_PASSTHRU, };
+/* Struct to hold the state and configuration of a 802.1qbg port */ +struct virNetlinkCallbackData { + 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 virNetlinkCallbackData *virNetlinkCallbackDataPtr; + +#define INSTANCE_STRLEN 36 + +static int instance2str(const unsigned char *p, char *dst, size_t size) +{ + if (dst && size > INSTANCE_STRLEN) { + snprintf(dst, size, "%02x%02x%02x%02x-%02x%02x-%02x%02x-" + "%02x%02x-%02x%02x%02x%02x%02x%02x", + p[0], p[1], p[2], p[3], + p[4], p[5], p[6], p[7], + p[8], p[9], p[10], p[11], p[12], p[13], p[14], p[15]); + return 0; + } + return -1; +} + +# define LLDPAD_PID_FILE "/var/run/lldpad.pid" +# define VIRIP_PID_FILE "/var/run/virip.pid" + +/** + * 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, + bool *handled, + void *opaque) +{ + struct nla_policy ifla_vf_policy[IFLA_VF_MAX + 1] = { + [IFLA_VF_MAC] = {.minlen = sizeof(struct ifla_vf_mac), + .maxlen = sizeof(struct ifla_vf_mac)}, + [IFLA_VF_VLAN] = {.minlen = sizeof(struct ifla_vf_vlan), + .maxlen = sizeof(struct ifla_vf_vlan)},
The indentation looks off here. Don't know if it's the actual patch or my mail client...
+ }; + + struct nla_policy ifla_port_policy[IFLA_PORT_MAX + 1] = { + [IFLA_PORT_RESPONSE] = {.type = NLA_U16}, + }; + + struct nlattr *tb[IFLA_MAX + 1], + *tb3[IFLA_PORT_MAX + 1], + *tb_vfinfo[IFLA_VF_MAX + 1], *tb_vfinfo_list; + + struct ifinfomsg ifinfo; + struct nlmsghdr *hdr; + void *data; + int rem; + char *ifname; + bool indicate = false;
The previous 4 lines all have tab characters instead of spaces. libvirt likes spaces only.
+ virNetlinkCallbackDataPtr calld = opaque; + pid_t lldpad_pid = 0; + pid_t virip_pid = 0; + + hdr = (struct nlmsghdr *) msg;
Another tab. I'll stop pointing these out, and just mention that "make syntax-check" will show these to you - you should try to run make syntax check before sending patches.
+ data = nlmsg_data(hdr); + + /* Quickly decide if we want this or not */ + + if (virPidFileReadPath(LLDPAD_PID_FILE, &lldpad_pid) < 0) + return; + + ignore_value(virPidFileReadPath(VIRIP_PID_FILE, &virip_pid)); + + if (hdr->nlmsg_pid != lldpad_pid && hdr->nlmsg_pid != virip_pid) + return; // we only care for lldpad and virip messages + if (hdr->nlmsg_type != RTM_SETLINK) + return; // we only care for RTM_SETLINK + if (*handled) + return; // if it has been handeled - dont handle again
misspelled "handled" (Could this if be further up in the function?)
+ + /* DEBUG start */ + VIR_INFO("netlink message nl_sockaddr: %p len: %d", peer, length); + VIR_DEBUG("nlmsg_type = 0x%02x",hdr->nlmsg_type); + VIR_DEBUG("nlmsg_len = 0x%04x",hdr->nlmsg_len ); + VIR_DEBUG("nlmsg_pid = %d",hdr->nlmsg_pid ); + VIR_DEBUG("nlmsg_seq = 0x%08x",hdr->nlmsg_seq ); + VIR_DEBUG("nlmsg_flags = 0x%04x",hdr->nlmsg_flags ); + + VIR_DEBUG("lldpad pid = %d",lldpad_pid); + + switch (hdr->nlmsg_type) { + case RTM_NEWLINK: + case RTM_DELLINK: + case RTM_SETLINK: + case RTM_GETLINK: + VIR_DEBUG(" IFINFOMSG\n"); + VIR_DEBUG(" ifi_family = 0x%02x\n", + ((struct ifinfomsg *)data)->ifi_family); + VIR_DEBUG(" ifi_type = 0x%x\n", + ((struct ifinfomsg *)data)->ifi_type); + VIR_DEBUG(" ifi_index = %i\n", + ((struct ifinfomsg *)data)->ifi_index); + VIR_DEBUG(" ifi_flags = 0x%04x\n", + ((struct ifinfomsg *)data)->ifi_flags); + VIR_DEBUG(" ifi_change = 0x%04x\n", + ((struct ifinfomsg *)data)->ifi_change); + } + /* DEBUG end */ + + /* Parse netlink message assume a setlink with vfports */ + memcpy(&ifinfo, NLMSG_DATA(hdr), sizeof ifinfo); + VIR_DEBUG("family:%#x type:%#x index:%d flags:%#x change:%#x", + ifinfo.ifi_family, ifinfo.ifi_type, ifinfo.ifi_index, + ifinfo.ifi_flags, ifinfo.ifi_change); + if (nlmsg_parse(hdr, sizeof ifinfo, + (struct nlattr **)&tb, IFLA_MAX, NULL)) { + VIR_DEBUG("error parsing request..."); + return; + } + + if (tb[IFLA_VFINFO_LIST]) { + VIR_DEBUG("FOUND IFLA_VFINFO_LIST!"); + + nla_for_each_nested(tb_vfinfo_list, tb[IFLA_VFINFO_LIST], rem) { + if (nla_type(tb_vfinfo_list) != IFLA_VF_INFO) { + VIR_DEBUG("nested parsing of" + "IFLA_VFINFO_LIST failed."); + return; + } + if (nla_parse_nested(tb_vfinfo, IFLA_VF_MAX, + tb_vfinfo_list, ifla_vf_policy)) { + VIR_DEBUG("nested parsing of " + "IFLA_VF_INFO failed."); + return; + } + } + + if (tb_vfinfo[IFLA_VF_MAC]) { + struct ifla_vf_mac *mac = + RTA_DATA(tb_vfinfo[IFLA_VF_MAC]); + unsigned char *m = mac->mac; + + VIR_DEBUG("IFLA_VF_MAC=%2x:%2x:%2x:%2x:%2x:%2x", + m[0], m[1], m[2], m[3], m[4], m[5]); + + if (memcmp(calld->macaddress, m, VIR_MAC_BUFLEN)) + { + /* Repeat the same check for a broadcast mac */ + unsigned char broadcastmac[VIR_MAC_BUFLEN]; + int i; + + for (i=0;i<VIR_MAC_BUFLEN;i++) + broadcastmac[i]=0xFF; + + if (memcmp(calld->macaddress, broadcastmac, VIR_MAC_BUFLEN)) { + VIR_DEBUG("MAC address match failed."); + return;
Why not just compare calld->macaddress[i] to 0xFF in the for loop, returning immediately when a non-FF byte is found?
+ } + } + } + + if (tb_vfinfo[IFLA_VF_VLAN]) { + struct ifla_vf_vlan *vlan = + RTA_DATA(tb_vfinfo[IFLA_VF_VLAN]); + + VIR_DEBUG("IFLA_VF_VLAN=%d", vlan->vlan); + } + } + + if (tb[IFLA_IFNAME]) { + ifname = (char *)RTA_DATA(tb[IFLA_IFNAME]); + VIR_DEBUG("IFLA_IFNAME=%s\n", ifname); + } + + if (tb[IFLA_OPERSTATE]) { + rem = *(unsigned short *)RTA_DATA(tb[IFLA_OPERSTATE]); + VIR_DEBUG("IFLA_OPERSTATE=%d\n", rem); + } + + if (tb[IFLA_VF_PORTS]) { + struct nlattr *tb_vf_ports; + + VIR_DEBUG("found IFLA_VF_PORTS\n"); + nla_for_each_nested(tb_vf_ports, tb[IFLA_VF_PORTS], rem) { + + VIR_DEBUG("iterating\n"); + if (nla_type(tb_vf_ports) != IFLA_VF_PORT) { + VIR_DEBUG("not a IFLA_VF_PORT. skipping\n"); + continue; + } + if (nla_parse_nested(tb3, IFLA_PORT_MAX, tb_vf_ports, + ifla_port_policy)) { + VIR_DEBUG("nested parsing on level 2" + " failed."); + } + if (tb3[IFLA_PORT_VF]) { + VIR_DEBUG("IFLA_PORT_VF=%d", + *(uint32_t + *) (RTA_DATA(tb3[IFLA_PORT_VF]))); + } + if (tb3[IFLA_PORT_PROFILE]) { + VIR_DEBUG("IFLA_PORT_PROFILE=%s", (char *) + RTA_DATA(tb3[IFLA_PORT_PROFILE])); + } + + if (tb3[IFLA_PORT_VSI_TYPE]) { + struct ifla_port_vsi *pvsi; + int tid = 0; + + pvsi = (struct ifla_port_vsi *) + RTA_DATA(tb3[IFLA_PORT_VSI_TYPE]); + tid = pvsi->vsi_type_id[2] << 16 | + pvsi->vsi_type_id[1] << 8 | + pvsi->vsi_type_id[0]; + + VIR_DEBUG("mgr_id: %d", pvsi->vsi_mgr_id); + VIR_DEBUG("type_id: %d", tid); + VIR_DEBUG("type_version: %d", + pvsi->vsi_type_version); + } + + if (tb3[IFLA_PORT_INSTANCE_UUID]) { + char instance[INSTANCE_STRLEN + 2]; + unsigned char *uuid; + + uuid = (unsigned char *) + RTA_DATA(tb3[IFLA_PORT_INSTANCE_UUID]); + instance2str(uuid, instance, sizeof(instance)); + VIR_DEBUG("IFLA_PORT_INSTANCE_UUID=%s\n", + instance); + } + + if (tb3[IFLA_PORT_REQUEST]) { + uint8_t req = *(uint8_t *) RTA_DATA(tb3[IFLA_PORT_REQUEST]); + VIR_DEBUG("IFLA_PORT_REQUEST=%d", req); + + if ( req == PORT_REQUEST_DISASSOCIATE ) { + VIR_DEBUG("Set dissaccociated."); + indicate=true; + } + } + + if (tb3[IFLA_PORT_RESPONSE]) { + VIR_DEBUG("IFLA_PORT_RESPONSE=%d\n", *(uint16_t *) + RTA_DATA(tb3[IFLA_PORT_RESPONSE])); + } + } + } + + if (!indicate) { + return; + } + + VIR_INFO("Re-send 802.1qbg associate request:"); + VIR_INFO(" if: %s",calld->cr_ifname ); + VIR_INFO(" lf: %s",calld->linkdev ); + VIR_INFO(" mac: %02x:%02x:%02x:%02x:%02x:%02x",calld->macaddress[0], + calld->macaddress[1], + calld->macaddress[2], + calld->macaddress[3], + calld->macaddress[4], + calld->macaddress[5] );
Bad indentation.
+ + ignore_value(virNetDevVPortProfileAssociate(calld->cr_ifname, + calld->virtPortProfile, + calld->macaddress, + calld->linkdev, + calld->vmuuid, calld->vmOp, true)); + + *handled = true; + return; +} + /** * virNetDevMacVLanCreateWithVPortProfile: * Create an instance of a macvtap device and open its tap character @@ -547,7 +838,7 @@ create_name: virtPortProfile, macaddress, linkdev, - vmuuid, vmOp) < 0) { + vmuuid, vmOp, false) < 0) { rc = -1; goto link_del_exit; } @@ -589,6 +880,23 @@ create_name: goto disassociate_exit; }
+ if (virNetlinkEventServiceIsRunning()) { + virNetlinkCallbackDataPtr calld; + + 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;
Put spaces around "=" signs.
+ + virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback, calld, macaddress);
Maybe we should check for a return value here.
+ }
return rc;
@@ -638,6 +946,9 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, if (virNetDevMacVLanDelete(ifname) < 0) ret = -1; } + + virNetlinkEventRemoveClient(0,macaddr); + return ret; }
diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index faadc3a..554f128 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -650,7 +650,8 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex, const unsigned char *instanceId, const unsigned char *hostUUID, int32_t vf, - uint8_t op) + uint8_t op, + bool setlink_only) { int rc; unsigned char *recvbuf = NULL; @@ -675,6 +676,9 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex, return rc; }
+ if (setlink_only) /*for re-associations on existing links*/ + return 0; + while (--repeats >= 0) { rc = virNetDevVPortProfileLinkDump(NULL, ifindex, nltarget_kernel, tb, &recvbuf, virNetDevVPortProfileGetLldpadPid); @@ -751,7 +755,8 @@ static int virNetDevVPortProfileOp8021Qbg(const char *ifname, const unsigned char *macaddr, const virNetDevVPortProfilePtr virtPort, - enum virNetDevVPortProfileLinkOp virtPortOp) + enum virNetDevVPortProfileLinkOp virtPortOp, + bool setlink_only) { int rc = 0; int op = PORT_REQUEST_ASSOCIATE; @@ -804,7 +809,8 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname, virtPort->u.virtPort8021Qbg.instanceID, NULL, vf, - op); + op, + setlink_only);
err_exit:
@@ -892,7 +898,8 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname, (virtPortOp == VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE_RR) ? PORT_REQUEST_PREASSOCIATE_RR - : PORT_REQUEST_ASSOCIATE); + : PORT_REQUEST_ASSOCIATE, + false); if (rc == -2) /* Association timed out, disassociate */ virNetDevVPortProfileOpCommon(NULL, ifindex, @@ -904,7 +911,8 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname, NULL, NULL, vf, - PORT_REQUEST_DISASSOCIATE); + PORT_REQUEST_DISASSOCIATE, + false); break;
case VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE: @@ -917,7 +925,8 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname, NULL, NULL, vf, - PORT_REQUEST_DISASSOCIATE); + PORT_REQUEST_DISASSOCIATE, + false); break;
default: @@ -938,6 +947,7 @@ err_exit: * @virtPort: pointer to the object holding port profile parameters * @vmuuid : the UUID of the virtual machine * @vmOp : The VM operation (i.e., create, no-op) + * @setlink_only : Only set the link - dont wait for the link to come up * * Associate a port on a swtich with a profile. This function * may notify a kernel driver or an external daemon to run @@ -954,7 +964,8 @@ virNetDevVPortProfileAssociate(const char *macvtap_ifname, const unsigned char *macvtap_macaddr, const char *linkdev, const unsigned char *vmuuid, - enum virNetDevVPortProfileOp vmOp) + enum virNetDevVPortProfileOp vmOp, + bool setlink_only) { int rc = 0;
@@ -976,7 +987,8 @@ virNetDevVPortProfileAssociate(const char *macvtap_ifname, virtPort, (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) ? VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE - : VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE); + : VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE, + setlink_only); break;
case VIR_NETDEV_VPORT_PROFILE_8021QBH: @@ -1033,7 +1045,7 @@ virNetDevVPortProfileDisassociate(const char *macvtap_ifname, case VIR_NETDEV_VPORT_PROFILE_8021QBG: rc = virNetDevVPortProfileOp8021Qbg(macvtap_ifname, macvtap_macaddr, virtPort, - VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE); + VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE, false); break;
case VIR_NETDEV_VPORT_PROFILE_8021QBH: @@ -1056,7 +1068,8 @@ int virNetDevVPortProfileAssociate(const char *macvtap_ifname ATTRIBUTE_UNUSED, const unsigned char *macvtap_macaddr ATTRIBUTE_UNUSED, const char *linkdev ATTRIBUTE_UNUSED, const unsigned char *vmuuid ATTRIBUTE_UNUSED, - enum virNetDevVPortProfileOp vmOp ATTRIBUTE_UNUSED) + enum virNetDevVPortProfileOp vmOp ATTRIBUTE_UNUSED, + bool setlink_only) { virReportSystemError(ENOSYS, "%s", _("Virtual port profile association not supported on this platform")); diff --git a/src/util/virnetdevvportprofile.h b/src/util/virnetdevvportprofile.h index 7a8d81f..fe80086 100644 --- a/src/util/virnetdevvportprofile.h +++ b/src/util/virnetdevvportprofile.h @@ -81,7 +81,8 @@ int virNetDevVPortProfileAssociate(const char *ifname, const unsigned char *macaddr, const char *linkdev, const unsigned char *vmuuid, - enum virNetDevVPortProfileOp vmOp) + enum virNetDevVPortProfileOp vmOp, + bool setlink_only) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK;

On Feb 23, 2012, at 11:16 PM, Laine Stump wrote: > On 02/22/2012 08:17 AM, D. Herrendoerfer wrote: >> From: "D. Herrendoerfer" <d.herrendoerfer@herrendoerfer.name> >> >> Add de-association handling for 802.1qbg (vepa) via lldpad >> netlink messages. Also adds the possibility to perform an >> association request without waiting for a confirmation. > > The main issue I see with this patch is with whitespace, but that can > easily be fixed prior to pushing it, so ACK. Is the message format > used > by this patch, the absolute final version? (i.e. can we safely push it > an know that it will be correct?) The patch to libvirt was picked this week. So yes, we can assume that the message will not be changed. But as always : Never say never ! The callback mechanism is not re-armed when libvirt is restarted now. The reason is: lldpad remembers who sent the associate by pid - since in theory there could be multiple agents performing associations. So if the libvirt pid changes, there is little we can do now. And thanks ! Dirk H > >> >> Signed-off-by: D. Herrendoerfer <d.herrendoerfer@herrendoerfer.name> >> --- >> src/qemu/qemu_migration.c | 2 +- >> src/util/virnetdevmacvlan.c | 315 ++++++++++++++++++++++++++++ >> +++++++++- >> src/util/virnetdevvportprofile.c | 33 +++-- >> src/util/virnetdevvportprofile.h | 3 +- >> 4 files changed, 339 insertions(+), 14 deletions(-) >> >> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c >> index 12cfbde..31428f8 100644 >> --- a/src/qemu/qemu_migration.c >> +++ b/src/qemu/qemu_migration.c >> @@ -2567,7 +2567,7 @@ >> qemuMigrationVPAssociatePortProfiles(virDomainDefPtr def) { >> net->mac, >> >> virDomainNetGetActualDirectDev(net), >> def->uuid, >> - >> VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH) < 0) >> + >> VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH, false) < 0) >> goto err_exit; >> } >> last_good_net = i; >> diff --git a/src/util/virnetdevmacvlan.c b/src/util/ >> virnetdevmacvlan.c >> index 25d0846..b3e3325 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 >> - > > spurious whitespace change. > >> # 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 >> @@ -68,6 +69,8 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, >> VIR_NETDEV_MACVLAN_MODE_LAST, >> # include "virfile.h" >> # include "virnetlink.h" >> # include "virnetdev.h" >> +# include "virpidfile.h" >> + >> >> # define MACVTAP_NAME_PREFIX "macvtap" >> # define MACVTAP_NAME_PATTERN "macvtap%d" >> @@ -75,6 +78,7 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, >> VIR_NETDEV_MACVLAN_MODE_LAST, >> # define MACVLAN_NAME_PREFIX "macvlan" >> # define MACVLAN_NAME_PATTERN "macvlan%d" >> >> + > > Another spurious whitespace change. > >> /** >> * virNetDevMacVLanCreate: >> * >> @@ -445,6 +449,293 @@ static const uint32_t >> modeMap[VIR_NETDEV_MACVLAN_MODE_LAST] = { >> [VIR_NETDEV_MACVLAN_MODE_PASSTHRU] = MACVLAN_MODE_PASSTHRU, >> }; >> >> +/* Struct to hold the state and configuration of a 802.1qbg port */ >> +struct virNetlinkCallbackData { >> + 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 virNetlinkCallbackData *virNetlinkCallbackDataPtr; >> + >> +#define INSTANCE_STRLEN 36 >> + >> +static int instance2str(const unsigned char *p, char *dst, size_t >> size) >> +{ >> + if (dst && size > INSTANCE_STRLEN) { >> + snprintf(dst, size, "%02x%02x%02x%02x-%02x%02x-%02x%02x-" >> + "%02x%02x-%02x%02x%02x%02x%02x%02x", >> + p[0], p[1], p[2], p[3], >> + p[4], p[5], p[6], p[7], >> + p[8], p[9], p[10], p[11], p[12], p[13], p[14], p[15]); >> + return 0; >> + } >> + return -1; >> +} >> + >> +# define LLDPAD_PID_FILE "/var/run/lldpad.pid" >> +# define VIRIP_PID_FILE "/var/run/virip.pid" >> + >> +/** >> + * 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, >> + bool *handled, >> + void *opaque) >> +{ >> + struct nla_policy ifla_vf_policy[IFLA_VF_MAX + 1] = { >> + [IFLA_VF_MAC] = {.minlen = sizeof(struct ifla_vf_mac), >> + .maxlen = sizeof(struct ifla_vf_mac)}, >> + [IFLA_VF_VLAN] = {.minlen = sizeof(struct ifla_vf_vlan), >> + .maxlen = sizeof(struct ifla_vf_vlan)}, > > The indentation looks off here. Don't know if it's the actual patch or > my mail client... > >> + }; >> + >> + struct nla_policy ifla_port_policy[IFLA_PORT_MAX + 1] = { >> + [IFLA_PORT_RESPONSE] = {.type = NLA_U16}, >> + }; >> + >> + struct nlattr *tb[IFLA_MAX + 1], >> + *tb3[IFLA_PORT_MAX + 1], >> + *tb_vfinfo[IFLA_VF_MAX + 1], *tb_vfinfo_list; >> + >> + struct ifinfomsg ifinfo; >> + struct nlmsghdr *hdr; >> + void *data; >> + int rem; >> + char *ifname; >> + bool indicate = false; > > The previous 4 lines all have tab characters instead of spaces. > libvirt > likes spaces only. > >> + virNetlinkCallbackDataPtr calld = opaque; >> + pid_t lldpad_pid = 0; >> + pid_t virip_pid = 0; >> + >> + hdr = (struct nlmsghdr *) msg; > > Another tab. I'll stop pointing these out, and just mention that "make > syntax-check" will show these to you - you should try to run make > syntax > check before sending patches. > >> + data = nlmsg_data(hdr); >> + >> + /* Quickly decide if we want this or not */ >> + >> + if (virPidFileReadPath(LLDPAD_PID_FILE, &lldpad_pid) < 0) >> + return; >> + >> + ignore_value(virPidFileReadPath(VIRIP_PID_FILE, &virip_pid)); >> + >> + if (hdr->nlmsg_pid != lldpad_pid && hdr->nlmsg_pid != virip_pid) >> + return; // we only care for lldpad and virip messages >> + if (hdr->nlmsg_type != RTM_SETLINK) >> + return; // we only care for RTM_SETLINK >> + if (*handled) >> + return; // if it has been handeled - dont handle again > > misspelled "handled" (Could this if be further up in the function?) > >> + >> + /* DEBUG start */ >> + VIR_INFO("netlink message nl_sockaddr: %p len: %d", peer, >> length); >> + VIR_DEBUG("nlmsg_type = 0x%02x",hdr->nlmsg_type); >> + VIR_DEBUG("nlmsg_len = 0x%04x",hdr->nlmsg_len ); >> + VIR_DEBUG("nlmsg_pid = %d",hdr->nlmsg_pid ); >> + VIR_DEBUG("nlmsg_seq = 0x%08x",hdr->nlmsg_seq ); >> + VIR_DEBUG("nlmsg_flags = 0x%04x",hdr->nlmsg_flags ); >> + >> + VIR_DEBUG("lldpad pid = %d",lldpad_pid); >> + >> + switch (hdr->nlmsg_type) { >> + case RTM_NEWLINK: >> + case RTM_DELLINK: >> + case RTM_SETLINK: >> + case RTM_GETLINK: >> + VIR_DEBUG(" IFINFOMSG\n"); >> + VIR_DEBUG(" ifi_family = 0x%02x\n", >> + ((struct ifinfomsg *)data)->ifi_family); >> + VIR_DEBUG(" ifi_type = 0x%x\n", >> + ((struct ifinfomsg *)data)->ifi_type); >> + VIR_DEBUG(" ifi_index = %i\n", >> + ((struct ifinfomsg *)data)->ifi_index); >> + VIR_DEBUG(" ifi_flags = 0x%04x\n", >> + ((struct ifinfomsg *)data)->ifi_flags); >> + VIR_DEBUG(" ifi_change = 0x%04x\n", >> + ((struct ifinfomsg *)data)->ifi_change); >> + } >> + /* DEBUG end */ >> + >> + /* Parse netlink message assume a setlink with vfports */ >> + memcpy(&ifinfo, NLMSG_DATA(hdr), sizeof ifinfo); >> + VIR_DEBUG("family:%#x type:%#x index:%d flags:%#x change:%#x", >> + ifinfo.ifi_family, ifinfo.ifi_type, ifinfo.ifi_index, >> + ifinfo.ifi_flags, ifinfo.ifi_change); >> + if (nlmsg_parse(hdr, sizeof ifinfo, >> + (struct nlattr **)&tb, IFLA_MAX, NULL)) { >> + VIR_DEBUG("error parsing request..."); >> + return; >> + } >> + >> + if (tb[IFLA_VFINFO_LIST]) { >> + VIR_DEBUG("FOUND IFLA_VFINFO_LIST!"); >> + >> + nla_for_each_nested(tb_vfinfo_list, tb[IFLA_VFINFO_LIST], >> rem) { >> + if (nla_type(tb_vfinfo_list) != IFLA_VF_INFO) { >> + VIR_DEBUG("nested parsing of" >> + "IFLA_VFINFO_LIST failed."); >> + return; >> + } >> + if (nla_parse_nested(tb_vfinfo, IFLA_VF_MAX, >> + tb_vfinfo_list, ifla_vf_policy)) { >> + VIR_DEBUG("nested parsing of " >> + "IFLA_VF_INFO failed."); >> + return; >> + } >> + } >> + >> + if (tb_vfinfo[IFLA_VF_MAC]) { >> + struct ifla_vf_mac *mac = >> + RTA_DATA(tb_vfinfo[IFLA_VF_MAC]); >> + unsigned char *m = mac->mac; >> + >> + VIR_DEBUG("IFLA_VF_MAC=%2x:%2x:%2x:%2x:%2x:%2x", >> + m[0], m[1], m[2], m[3], m[4], m[5]); >> + >> + if (memcmp(calld->macaddress, m, VIR_MAC_BUFLEN)) >> + { >> + /* Repeat the same check for a broadcast mac */ >> + unsigned char broadcastmac[VIR_MAC_BUFLEN]; >> + int i; >> + >> + for (i=0;i<VIR_MAC_BUFLEN;i++) >> + broadcastmac[i]=0xFF; >> + >> + if (memcmp(calld->macaddress, broadcastmac, >> VIR_MAC_BUFLEN)) { >> + VIR_DEBUG("MAC address match failed."); >> + return; > > > Why not just compare calld->macaddress[i] to 0xFF in the for loop, > returning immediately when a non-FF byte is found? > > >> + } >> + } >> + } >> + >> + if (tb_vfinfo[IFLA_VF_VLAN]) { >> + struct ifla_vf_vlan *vlan = >> + RTA_DATA(tb_vfinfo[IFLA_VF_VLAN]); >> + >> + VIR_DEBUG("IFLA_VF_VLAN=%d", vlan->vlan); >> + } >> + } >> + >> + if (tb[IFLA_IFNAME]) { >> + ifname = (char *)RTA_DATA(tb[IFLA_IFNAME]); >> + VIR_DEBUG("IFLA_IFNAME=%s\n", ifname); >> + } >> + >> + if (tb[IFLA_OPERSTATE]) { >> + rem = *(unsigned short *)RTA_DATA(tb[IFLA_OPERSTATE]); >> + VIR_DEBUG("IFLA_OPERSTATE=%d\n", rem); >> + } >> + >> + if (tb[IFLA_VF_PORTS]) { >> + struct nlattr *tb_vf_ports; >> + >> + VIR_DEBUG("found IFLA_VF_PORTS\n"); >> + nla_for_each_nested(tb_vf_ports, tb[IFLA_VF_PORTS], rem) { >> + >> + VIR_DEBUG("iterating\n"); >> + if (nla_type(tb_vf_ports) != IFLA_VF_PORT) { >> + VIR_DEBUG("not a IFLA_VF_PORT. skipping\n"); >> + continue; >> + } >> + if (nla_parse_nested(tb3, IFLA_PORT_MAX, tb_vf_ports, >> + ifla_port_policy)) { >> + VIR_DEBUG("nested parsing on level 2" >> + " failed."); >> + } >> + if (tb3[IFLA_PORT_VF]) { >> + VIR_DEBUG("IFLA_PORT_VF=%d", >> + *(uint32_t >> + *) (RTA_DATA(tb3[IFLA_PORT_VF]))); >> + } >> + if (tb3[IFLA_PORT_PROFILE]) { >> + VIR_DEBUG("IFLA_PORT_PROFILE=%s", (char *) >> + RTA_DATA(tb3[IFLA_PORT_PROFILE])); >> + } >> + >> + if (tb3[IFLA_PORT_VSI_TYPE]) { >> + struct ifla_port_vsi *pvsi; >> + int tid = 0; >> + >> + pvsi = (struct ifla_port_vsi *) >> + RTA_DATA(tb3[IFLA_PORT_VSI_TYPE]); >> + tid = pvsi->vsi_type_id[2] << 16 | >> + pvsi->vsi_type_id[1] << 8 | >> + pvsi->vsi_type_id[0]; >> + >> + VIR_DEBUG("mgr_id: %d", pvsi->vsi_mgr_id); >> + VIR_DEBUG("type_id: %d", tid); >> + VIR_DEBUG("type_version: %d", >> + pvsi->vsi_type_version); >> + } >> + >> + if (tb3[IFLA_PORT_INSTANCE_UUID]) { >> + char instance[INSTANCE_STRLEN + 2]; >> + unsigned char *uuid; >> + >> + uuid = (unsigned char *) >> + RTA_DATA(tb3[IFLA_PORT_INSTANCE_UUID]); >> + instance2str(uuid, instance, sizeof(instance)); >> + VIR_DEBUG("IFLA_PORT_INSTANCE_UUID=%s\n", >> + instance); >> + } >> + >> + if (tb3[IFLA_PORT_REQUEST]) { >> + uint8_t req = *(uint8_t *) >> RTA_DATA(tb3[IFLA_PORT_REQUEST]); >> + VIR_DEBUG("IFLA_PORT_REQUEST=%d", req); >> + >> + if ( req == PORT_REQUEST_DISASSOCIATE ) { >> + VIR_DEBUG("Set dissaccociated."); >> + indicate=true; >> + } >> + } >> + >> + if (tb3[IFLA_PORT_RESPONSE]) { >> + VIR_DEBUG("IFLA_PORT_RESPONSE=%d\n", *(uint16_t *) >> + RTA_DATA(tb3[IFLA_PORT_RESPONSE])); >> + } >> + } >> + } >> + >> + if (!indicate) { >> + return; >> + } >> + >> + VIR_INFO("Re-send 802.1qbg associate request:"); >> + VIR_INFO(" if: %s",calld->cr_ifname ); >> + VIR_INFO(" lf: %s",calld->linkdev ); >> + VIR_INFO(" mac: %02x:%02x:%02x:%02x:%02x:%02x",calld- >> >macaddress[0], >> + calld- >> >macaddress[1], >> + calld- >> >macaddress[2], >> + calld- >> >macaddress[3], >> + calld- >> >macaddress[4], >> + calld- >> >macaddress[5] ); > > Bad indentation. > >> + >> + ignore_value(virNetDevVPortProfileAssociate(calld->cr_ifname, >> + calld->virtPortProfile, >> + calld->macaddress, >> + calld->linkdev, >> + calld->vmuuid, calld- >> >vmOp, true)); >> + >> + *handled = true; >> + return; >> +} >> + >> /** >> * virNetDevMacVLanCreateWithVPortProfile: >> * Create an instance of a macvtap device and open its tap character >> @@ -547,7 +838,7 @@ create_name: >> virtPortProfile, >> macaddress, >> linkdev, >> - vmuuid, vmOp) < 0) { >> + vmuuid, vmOp, false) < 0) { >> rc = -1; >> goto link_del_exit; >> } >> @@ -589,6 +880,23 @@ create_name: >> goto disassociate_exit; >> } >> >> + if (virNetlinkEventServiceIsRunning()) { >> + virNetlinkCallbackDataPtr calld; >> + >> + 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; > > Put spaces around "=" signs. > >> + >> + >> virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback, >> calld, macaddress); > > Maybe we should check for a return value here. > > >> + } >> >> return rc; >> >> @@ -638,6 +946,9 @@ int >> virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, >> if (virNetDevMacVLanDelete(ifname) < 0) >> ret = -1; >> } >> + >> + virNetlinkEventRemoveClient(0,macaddr); >> + >> return ret; >> } >> >> diff --git a/src/util/virnetdevvportprofile.c b/src/util/ >> virnetdevvportprofile.c >> index faadc3a..554f128 100644 >> --- a/src/util/virnetdevvportprofile.c >> +++ b/src/util/virnetdevvportprofile.c >> @@ -650,7 +650,8 @@ virNetDevVPortProfileOpCommon(const char >> *ifname, int ifindex, >> const unsigned char *instanceId, >> const unsigned char *hostUUID, >> int32_t vf, >> - uint8_t op) >> + uint8_t op, >> + bool setlink_only) >> { >> int rc; >> unsigned char *recvbuf = NULL; >> @@ -675,6 +676,9 @@ virNetDevVPortProfileOpCommon(const char >> *ifname, int ifindex, >> return rc; >> } >> >> + if (setlink_only) /*for re-associations on existing links*/ >> + return 0; >> + >> while (--repeats >= 0) { >> rc = virNetDevVPortProfileLinkDump(NULL, ifindex, >> nltarget_kernel, tb, >> &recvbuf, >> virNetDevVPortProfileGetLldpadPid); >> @@ -751,7 +755,8 @@ static int >> virNetDevVPortProfileOp8021Qbg(const char *ifname, >> const unsigned char *macaddr, >> const virNetDevVPortProfilePtr >> virtPort, >> - enum virNetDevVPortProfileLinkOp >> virtPortOp) >> + enum virNetDevVPortProfileLinkOp >> virtPortOp, >> + bool setlink_only) >> { >> int rc = 0; >> int op = PORT_REQUEST_ASSOCIATE; >> @@ -804,7 +809,8 @@ virNetDevVPortProfileOp8021Qbg(const char >> *ifname, >> virtPort- >> >u.virtPort8021Qbg.instanceID, >> NULL, >> vf, >> - op); >> + op, >> + setlink_only); >> >> err_exit: >> >> @@ -892,7 +898,8 @@ virNetDevVPortProfileOp8021Qbh(const char >> *ifname, >> (virtPortOp == >> >> VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE_RR) ? >> >> PORT_REQUEST_PREASSOCIATE_RR >> - : >> PORT_REQUEST_ASSOCIATE); >> + : PORT_REQUEST_ASSOCIATE, >> + false); >> if (rc == -2) >> /* Association timed out, disassociate */ >> virNetDevVPortProfileOpCommon(NULL, ifindex, >> @@ -904,7 +911,8 @@ virNetDevVPortProfileOp8021Qbh(const char >> *ifname, >> NULL, >> NULL, >> vf, >> - >> PORT_REQUEST_DISASSOCIATE); >> + PORT_REQUEST_DISASSOCIATE, >> + false); >> break; >> >> case VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE: >> @@ -917,7 +925,8 @@ virNetDevVPortProfileOp8021Qbh(const char >> *ifname, >> NULL, >> NULL, >> vf, >> - >> PORT_REQUEST_DISASSOCIATE); >> + >> PORT_REQUEST_DISASSOCIATE, >> + false); >> break; >> >> default: >> @@ -938,6 +947,7 @@ err_exit: >> * @virtPort: pointer to the object holding port profile parameters >> * @vmuuid : the UUID of the virtual machine >> * @vmOp : The VM operation (i.e., create, no-op) >> + * @setlink_only : Only set the link - dont wait for the link to >> come up >> * >> * Associate a port on a swtich with a profile. This function >> * may notify a kernel driver or an external daemon to run >> @@ -954,7 +964,8 @@ virNetDevVPortProfileAssociate(const char >> *macvtap_ifname, >> const unsigned char *macvtap_macaddr, >> const char *linkdev, >> const unsigned char *vmuuid, >> - enum virNetDevVPortProfileOp vmOp) >> + enum virNetDevVPortProfileOp vmOp, >> + bool setlink_only) >> { >> int rc = 0; >> >> @@ -976,7 +987,8 @@ virNetDevVPortProfileAssociate(const char >> *macvtap_ifname, >> virtPort, >> (vmOp == >> VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) >> ? >> VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE >> - : >> VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE); >> + : >> VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE, >> + setlink_only); >> break; >> >> case VIR_NETDEV_VPORT_PROFILE_8021QBH: >> @@ -1033,7 +1045,7 @@ virNetDevVPortProfileDisassociate(const char >> *macvtap_ifname, >> case VIR_NETDEV_VPORT_PROFILE_8021QBG: >> rc = virNetDevVPortProfileOp8021Qbg(macvtap_ifname, >> macvtap_macaddr, >> virtPort, >> - >> VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE); >> + >> VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE, false); >> break; >> >> case VIR_NETDEV_VPORT_PROFILE_8021QBH: >> @@ -1056,7 +1068,8 @@ int virNetDevVPortProfileAssociate(const char >> *macvtap_ifname ATTRIBUTE_UNUSED, >> const unsigned char *macvtap_macaddr >> ATTRIBUTE_UNUSED, >> const char *linkdev ATTRIBUTE_UNUSED, >> const unsigned char *vmuuid >> ATTRIBUTE_UNUSED, >> - enum virNetDevVPortProfileOp vmOp >> ATTRIBUTE_UNUSED) >> + enum virNetDevVPortProfileOp vmOp >> ATTRIBUTE_UNUSED, >> + bool setlink_only) >> { >> virReportSystemError(ENOSYS, "%s", >> _("Virtual port profile association not >> supported on this platform")); >> diff --git a/src/util/virnetdevvportprofile.h b/src/util/ >> virnetdevvportprofile.h >> index 7a8d81f..fe80086 100644 >> --- a/src/util/virnetdevvportprofile.h >> +++ b/src/util/virnetdevvportprofile.h >> @@ -81,7 +81,8 @@ int virNetDevVPortProfileAssociate(const char >> *ifname, >> const unsigned char *macaddr, >> const char *linkdev, >> const unsigned char *vmuuid, >> - enum virNetDevVPortProfileOp >> vmOp) >> + enum virNetDevVPortProfileOp >> vmOp, >> + bool setlink_only) >> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) >> ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; >> > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list

On Feb 24, 2012, at 3:29 PM, D. Herrendoerfer wrote: > > On Feb 23, 2012, at 11:16 PM, Laine Stump wrote: > >> On 02/22/2012 08:17 AM, D. Herrendoerfer wrote: >>> From: "D. Herrendoerfer" <d.herrendoerfer@herrendoerfer.name> >>> >>> Add de-association handling for 802.1qbg (vepa) via lldpad >>> netlink messages. Also adds the possibility to perform an >>> association request without waiting for a confirmation. >> >> The main issue I see with this patch is with whitespace, but that can >> easily be fixed prior to pushing it, so ACK. Is the message format >> used >> by this patch, the absolute final version? (i.e. can we safely push >> it >> an know that it will be correct?) > > The patch to libvirt was picked this week. So yes, we can assume > that the > message will not be changed. But as always : Never say never ! The patch to lldpad (Doh!) was picked ... > The callback mechanism is not re-armed when libvirt is restarted now. > The reason is: lldpad remembers who sent the associate by pid - since > in theory there could be multiple agents performing associations. > So if the libvirt pid changes, there is little we can do now. > > And thanks ! > > Dirk H > >> >>> >>> Signed-off-by: D. Herrendoerfer <d.herrendoerfer@herrendoerfer.name> >>> --- >>> src/qemu/qemu_migration.c | 2 +- >>> src/util/virnetdevmacvlan.c | 315 +++++++++++++++++++++++++++ >>> ++++++++++- >>> src/util/virnetdevvportprofile.c | 33 +++-- >>> src/util/virnetdevvportprofile.h | 3 +- >>> 4 files changed, 339 insertions(+), 14 deletions(-) >>> >>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c >>> index 12cfbde..31428f8 100644 >>> --- a/src/qemu/qemu_migration.c >>> +++ b/src/qemu/qemu_migration.c >>> @@ -2567,7 +2567,7 @@ >>> qemuMigrationVPAssociatePortProfiles(virDomainDefPtr def) { >>> net->mac, >>> >>> virDomainNetGetActualDirectDev(net), >>> def->uuid, >>> - >>> VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH) < 0) >>> + >>> VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH, false) < 0) >>> goto err_exit; >>> } >>> last_good_net = i; >>> diff --git a/src/util/virnetdevmacvlan.c b/src/util/ >>> virnetdevmacvlan.c >>> index 25d0846..b3e3325 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 >>> - >> >> spurious whitespace change. >> >>> # 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 >>> @@ -68,6 +69,8 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, >>> VIR_NETDEV_MACVLAN_MODE_LAST, >>> # include "virfile.h" >>> # include "virnetlink.h" >>> # include "virnetdev.h" >>> +# include "virpidfile.h" >>> + >>> >>> # define MACVTAP_NAME_PREFIX "macvtap" >>> # define MACVTAP_NAME_PATTERN "macvtap%d" >>> @@ -75,6 +78,7 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, >>> VIR_NETDEV_MACVLAN_MODE_LAST, >>> # define MACVLAN_NAME_PREFIX "macvlan" >>> # define MACVLAN_NAME_PATTERN "macvlan%d" >>> >>> + >> >> Another spurious whitespace change. >> >>> /** >>> * virNetDevMacVLanCreate: >>> * >>> @@ -445,6 +449,293 @@ static const uint32_t >>> modeMap[VIR_NETDEV_MACVLAN_MODE_LAST] = { >>> [VIR_NETDEV_MACVLAN_MODE_PASSTHRU] = MACVLAN_MODE_PASSTHRU, >>> }; >>> >>> +/* Struct to hold the state and configuration of a 802.1qbg port */ >>> +struct virNetlinkCallbackData { >>> + 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 virNetlinkCallbackData *virNetlinkCallbackDataPtr; >>> + >>> +#define INSTANCE_STRLEN 36 >>> + >>> +static int instance2str(const unsigned char *p, char *dst, size_t >>> size) >>> +{ >>> + if (dst && size > INSTANCE_STRLEN) { >>> + snprintf(dst, size, "%02x%02x%02x%02x-%02x%02x-%02x%02x-" >>> + "%02x%02x-%02x%02x%02x%02x%02x%02x", >>> + p[0], p[1], p[2], p[3], >>> + p[4], p[5], p[6], p[7], >>> + p[8], p[9], p[10], p[11], p[12], p[13], p[14], p[15]); >>> + return 0; >>> + } >>> + return -1; >>> +} >>> + >>> +# define LLDPAD_PID_FILE "/var/run/lldpad.pid" >>> +# define VIRIP_PID_FILE "/var/run/virip.pid" >>> + >>> +/** >>> + * 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, >>> + bool *handled, >>> + void *opaque) >>> +{ >>> + struct nla_policy ifla_vf_policy[IFLA_VF_MAX + 1] = { >>> + [IFLA_VF_MAC] = {.minlen = sizeof(struct ifla_vf_mac), >>> + .maxlen = sizeof(struct ifla_vf_mac)}, >>> + [IFLA_VF_VLAN] = {.minlen = sizeof(struct ifla_vf_vlan), >>> + .maxlen = sizeof(struct ifla_vf_vlan)}, >> >> The indentation looks off here. Don't know if it's the actual patch >> or >> my mail client... >> >>> + }; >>> + >>> + struct nla_policy ifla_port_policy[IFLA_PORT_MAX + 1] = { >>> + [IFLA_PORT_RESPONSE] = {.type = NLA_U16}, >>> + }; >>> + >>> + struct nlattr *tb[IFLA_MAX + 1], >>> + *tb3[IFLA_PORT_MAX + 1], >>> + *tb_vfinfo[IFLA_VF_MAX + 1], *tb_vfinfo_list; >>> + >>> + struct ifinfomsg ifinfo; >>> + struct nlmsghdr *hdr; >>> + void *data; >>> + int rem; >>> + char *ifname; >>> + bool indicate = false; >> >> The previous 4 lines all have tab characters instead of spaces. >> libvirt >> likes spaces only. >> >>> + virNetlinkCallbackDataPtr calld = opaque; >>> + pid_t lldpad_pid = 0; >>> + pid_t virip_pid = 0; >>> + >>> + hdr = (struct nlmsghdr *) msg; >> >> Another tab. I'll stop pointing these out, and just mention that >> "make >> syntax-check" will show these to you - you should try to run make >> syntax >> check before sending patches. >> >>> + data = nlmsg_data(hdr); >>> + >>> + /* Quickly decide if we want this or not */ >>> + >>> + if (virPidFileReadPath(LLDPAD_PID_FILE, &lldpad_pid) < 0) >>> + return; >>> + >>> + ignore_value(virPidFileReadPath(VIRIP_PID_FILE, &virip_pid)); >>> + >>> + if (hdr->nlmsg_pid != lldpad_pid && hdr->nlmsg_pid != >>> virip_pid) >>> + return; // we only care for lldpad and virip messages >>> + if (hdr->nlmsg_type != RTM_SETLINK) >>> + return; // we only care for RTM_SETLINK >>> + if (*handled) >>> + return; // if it has been handeled - dont handle again >> >> misspelled "handled" (Could this if be further up in the function?) >> >>> + >>> + /* DEBUG start */ >>> + VIR_INFO("netlink message nl_sockaddr: %p len: %d", peer, >>> length); >>> + VIR_DEBUG("nlmsg_type = 0x%02x",hdr->nlmsg_type); >>> + VIR_DEBUG("nlmsg_len = 0x%04x",hdr->nlmsg_len ); >>> + VIR_DEBUG("nlmsg_pid = %d",hdr->nlmsg_pid ); >>> + VIR_DEBUG("nlmsg_seq = 0x%08x",hdr->nlmsg_seq ); >>> + VIR_DEBUG("nlmsg_flags = 0x%04x",hdr->nlmsg_flags ); >>> + >>> + VIR_DEBUG("lldpad pid = %d",lldpad_pid); >>> + >>> + switch (hdr->nlmsg_type) { >>> + case RTM_NEWLINK: >>> + case RTM_DELLINK: >>> + case RTM_SETLINK: >>> + case RTM_GETLINK: >>> + VIR_DEBUG(" IFINFOMSG\n"); >>> + VIR_DEBUG(" ifi_family = 0x%02x\n", >>> + ((struct ifinfomsg *)data)->ifi_family); >>> + VIR_DEBUG(" ifi_type = 0x%x\n", >>> + ((struct ifinfomsg *)data)->ifi_type); >>> + VIR_DEBUG(" ifi_index = %i\n", >>> + ((struct ifinfomsg *)data)->ifi_index); >>> + VIR_DEBUG(" ifi_flags = 0x%04x\n", >>> + ((struct ifinfomsg *)data)->ifi_flags); >>> + VIR_DEBUG(" ifi_change = 0x%04x\n", >>> + ((struct ifinfomsg *)data)->ifi_change); >>> + } >>> + /* DEBUG end */ >>> + >>> + /* Parse netlink message assume a setlink with vfports */ >>> + memcpy(&ifinfo, NLMSG_DATA(hdr), sizeof ifinfo); >>> + VIR_DEBUG("family:%#x type:%#x index:%d flags:%#x change:%#x", >>> + ifinfo.ifi_family, ifinfo.ifi_type, ifinfo.ifi_index, >>> + ifinfo.ifi_flags, ifinfo.ifi_change); >>> + if (nlmsg_parse(hdr, sizeof ifinfo, >>> + (struct nlattr **)&tb, IFLA_MAX, NULL)) { >>> + VIR_DEBUG("error parsing request..."); >>> + return; >>> + } >>> + >>> + if (tb[IFLA_VFINFO_LIST]) { >>> + VIR_DEBUG("FOUND IFLA_VFINFO_LIST!"); >>> + >>> + nla_for_each_nested(tb_vfinfo_list, tb[IFLA_VFINFO_LIST], >>> rem) { >>> + if (nla_type(tb_vfinfo_list) != IFLA_VF_INFO) { >>> + VIR_DEBUG("nested parsing of" >>> + "IFLA_VFINFO_LIST failed."); >>> + return; >>> + } >>> + if (nla_parse_nested(tb_vfinfo, IFLA_VF_MAX, >>> + tb_vfinfo_list, ifla_vf_policy)) { >>> + VIR_DEBUG("nested parsing of " >>> + "IFLA_VF_INFO failed."); >>> + return; >>> + } >>> + } >>> + >>> + if (tb_vfinfo[IFLA_VF_MAC]) { >>> + struct ifla_vf_mac *mac = >>> + RTA_DATA(tb_vfinfo[IFLA_VF_MAC]); >>> + unsigned char *m = mac->mac; >>> + >>> + VIR_DEBUG("IFLA_VF_MAC=%2x:%2x:%2x:%2x:%2x:%2x", >>> + m[0], m[1], m[2], m[3], m[4], m[5]); >>> + >>> + if (memcmp(calld->macaddress, m, VIR_MAC_BUFLEN)) >>> + { >>> + /* Repeat the same check for a broadcast mac */ >>> + unsigned char broadcastmac[VIR_MAC_BUFLEN]; >>> + int i; >>> + >>> + for (i=0;i<VIR_MAC_BUFLEN;i++) >>> + broadcastmac[i]=0xFF; >>> + >>> + if (memcmp(calld->macaddress, broadcastmac, >>> VIR_MAC_BUFLEN)) { >>> + VIR_DEBUG("MAC address match failed."); >>> + return; >> >> >> Why not just compare calld->macaddress[i] to 0xFF in the for loop, >> returning immediately when a non-FF byte is found? >> >> >>> + } >>> + } >>> + } >>> + >>> + if (tb_vfinfo[IFLA_VF_VLAN]) { >>> + struct ifla_vf_vlan *vlan = >>> + RTA_DATA(tb_vfinfo[IFLA_VF_VLAN]); >>> + >>> + VIR_DEBUG("IFLA_VF_VLAN=%d", vlan->vlan); >>> + } >>> + } >>> + >>> + if (tb[IFLA_IFNAME]) { >>> + ifname = (char *)RTA_DATA(tb[IFLA_IFNAME]); >>> + VIR_DEBUG("IFLA_IFNAME=%s\n", ifname); >>> + } >>> + >>> + if (tb[IFLA_OPERSTATE]) { >>> + rem = *(unsigned short *)RTA_DATA(tb[IFLA_OPERSTATE]); >>> + VIR_DEBUG("IFLA_OPERSTATE=%d\n", rem); >>> + } >>> + >>> + if (tb[IFLA_VF_PORTS]) { >>> + struct nlattr *tb_vf_ports; >>> + >>> + VIR_DEBUG("found IFLA_VF_PORTS\n"); >>> + nla_for_each_nested(tb_vf_ports, tb[IFLA_VF_PORTS], rem) { >>> + >>> + VIR_DEBUG("iterating\n"); >>> + if (nla_type(tb_vf_ports) != IFLA_VF_PORT) { >>> + VIR_DEBUG("not a IFLA_VF_PORT. skipping\n"); >>> + continue; >>> + } >>> + if (nla_parse_nested(tb3, IFLA_PORT_MAX, tb_vf_ports, >>> + ifla_port_policy)) { >>> + VIR_DEBUG("nested parsing on level 2" >>> + " failed."); >>> + } >>> + if (tb3[IFLA_PORT_VF]) { >>> + VIR_DEBUG("IFLA_PORT_VF=%d", >>> + *(uint32_t >>> + *) (RTA_DATA(tb3[IFLA_PORT_VF]))); >>> + } >>> + if (tb3[IFLA_PORT_PROFILE]) { >>> + VIR_DEBUG("IFLA_PORT_PROFILE=%s", (char *) >>> + RTA_DATA(tb3[IFLA_PORT_PROFILE])); >>> + } >>> + >>> + if (tb3[IFLA_PORT_VSI_TYPE]) { >>> + struct ifla_port_vsi *pvsi; >>> + int tid = 0; >>> + >>> + pvsi = (struct ifla_port_vsi *) >>> + RTA_DATA(tb3[IFLA_PORT_VSI_TYPE]); >>> + tid = pvsi->vsi_type_id[2] << 16 | >>> + pvsi->vsi_type_id[1] << 8 | >>> + pvsi->vsi_type_id[0]; >>> + >>> + VIR_DEBUG("mgr_id: %d", pvsi->vsi_mgr_id); >>> + VIR_DEBUG("type_id: %d", tid); >>> + VIR_DEBUG("type_version: %d", >>> + pvsi->vsi_type_version); >>> + } >>> + >>> + if (tb3[IFLA_PORT_INSTANCE_UUID]) { >>> + char instance[INSTANCE_STRLEN + 2]; >>> + unsigned char *uuid; >>> + >>> + uuid = (unsigned char *) >>> + RTA_DATA(tb3[IFLA_PORT_INSTANCE_UUID]); >>> + instance2str(uuid, instance, sizeof(instance)); >>> + VIR_DEBUG("IFLA_PORT_INSTANCE_UUID=%s\n", >>> + instance); >>> + } >>> + >>> + if (tb3[IFLA_PORT_REQUEST]) { >>> + uint8_t req = *(uint8_t *) >>> RTA_DATA(tb3[IFLA_PORT_REQUEST]); >>> + VIR_DEBUG("IFLA_PORT_REQUEST=%d", req); >>> + >>> + if ( req == PORT_REQUEST_DISASSOCIATE ) { >>> + VIR_DEBUG("Set dissaccociated."); >>> + indicate=true; >>> + } >>> + } >>> + >>> + if (tb3[IFLA_PORT_RESPONSE]) { >>> + VIR_DEBUG("IFLA_PORT_RESPONSE=%d\n", *(uint16_t *) >>> + RTA_DATA(tb3[IFLA_PORT_RESPONSE])); >>> + } >>> + } >>> + } >>> + >>> + if (!indicate) { >>> + return; >>> + } >>> + >>> + VIR_INFO("Re-send 802.1qbg associate request:"); >>> + VIR_INFO(" if: %s",calld->cr_ifname ); >>> + VIR_INFO(" lf: %s",calld->linkdev ); >>> + VIR_INFO(" mac: %02x:%02x:%02x:%02x:%02x:%02x",calld- >>> >macaddress[0], >>> + calld- >>> >macaddress[1], >>> + calld- >>> >macaddress[2], >>> + calld- >>> >macaddress[3], >>> + calld- >>> >macaddress[4], >>> + calld- >>> >macaddress[5] ); >> >> Bad indentation. >> >>> + >>> + ignore_value(virNetDevVPortProfileAssociate(calld->cr_ifname, >>> + calld->virtPortProfile, >>> + calld->macaddress, >>> + calld->linkdev, >>> + calld->vmuuid, calld- >>> >vmOp, true)); >>> + >>> + *handled = true; >>> + return; >>> +} >>> + >>> /** >>> * virNetDevMacVLanCreateWithVPortProfile: >>> * Create an instance of a macvtap device and open its tap character >>> @@ -547,7 +838,7 @@ create_name: >>> virtPortProfile, >>> macaddress, >>> linkdev, >>> - vmuuid, vmOp) < 0) { >>> + vmuuid, vmOp, false) < 0) { >>> rc = -1; >>> goto link_del_exit; >>> } >>> @@ -589,6 +880,23 @@ create_name: >>> goto disassociate_exit; >>> } >>> >>> + if (virNetlinkEventServiceIsRunning()) { >>> + virNetlinkCallbackDataPtr calld; >>> + >>> + 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; >> >> Put spaces around "=" signs. >> >>> + >>> + >>> virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback, >>> calld, macaddress); >> >> Maybe we should check for a return value here. >> >> >>> + } >>> >>> return rc; >>> >>> @@ -638,6 +946,9 @@ int >>> virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, >>> if (virNetDevMacVLanDelete(ifname) < 0) >>> ret = -1; >>> } >>> + >>> + virNetlinkEventRemoveClient(0,macaddr); >>> + >>> return ret; >>> } >>> >>> diff --git a/src/util/virnetdevvportprofile.c b/src/util/ >>> virnetdevvportprofile.c >>> index faadc3a..554f128 100644 >>> --- a/src/util/virnetdevvportprofile.c >>> +++ b/src/util/virnetdevvportprofile.c >>> @@ -650,7 +650,8 @@ virNetDevVPortProfileOpCommon(const char >>> *ifname, int ifindex, >>> const unsigned char *instanceId, >>> const unsigned char *hostUUID, >>> int32_t vf, >>> - uint8_t op) >>> + uint8_t op, >>> + bool setlink_only) >>> { >>> int rc; >>> unsigned char *recvbuf = NULL; >>> @@ -675,6 +676,9 @@ virNetDevVPortProfileOpCommon(const char >>> *ifname, int ifindex, >>> return rc; >>> } >>> >>> + if (setlink_only) /*for re-associations on existing links*/ >>> + return 0; >>> + >>> while (--repeats >= 0) { >>> rc = virNetDevVPortProfileLinkDump(NULL, ifindex, >>> nltarget_kernel, tb, >>> &recvbuf, >>> virNetDevVPortProfileGetLldpadPid); >>> @@ -751,7 +755,8 @@ static int >>> virNetDevVPortProfileOp8021Qbg(const char *ifname, >>> const unsigned char *macaddr, >>> const virNetDevVPortProfilePtr >>> virtPort, >>> - enum virNetDevVPortProfileLinkOp >>> virtPortOp) >>> + enum virNetDevVPortProfileLinkOp >>> virtPortOp, >>> + bool setlink_only) >>> { >>> int rc = 0; >>> int op = PORT_REQUEST_ASSOCIATE; >>> @@ -804,7 +809,8 @@ virNetDevVPortProfileOp8021Qbg(const char >>> *ifname, >>> virtPort- >>> >u.virtPort8021Qbg.instanceID, >>> NULL, >>> vf, >>> - op); >>> + op, >>> + setlink_only); >>> >>> err_exit: >>> >>> @@ -892,7 +898,8 @@ virNetDevVPortProfileOp8021Qbh(const char >>> *ifname, >>> (virtPortOp == >>> >>> VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE_RR) ? >>> >>> PORT_REQUEST_PREASSOCIATE_RR >>> - : >>> PORT_REQUEST_ASSOCIATE); >>> + : >>> PORT_REQUEST_ASSOCIATE, >>> + false); >>> if (rc == -2) >>> /* Association timed out, disassociate */ >>> virNetDevVPortProfileOpCommon(NULL, ifindex, >>> @@ -904,7 +911,8 @@ virNetDevVPortProfileOp8021Qbh(const char >>> *ifname, >>> NULL, >>> NULL, >>> vf, >>> - >>> PORT_REQUEST_DISASSOCIATE); >>> + >>> PORT_REQUEST_DISASSOCIATE, >>> + false); >>> break; >>> >>> case VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE: >>> @@ -917,7 +925,8 @@ virNetDevVPortProfileOp8021Qbh(const char >>> *ifname, >>> NULL, >>> NULL, >>> vf, >>> - >>> PORT_REQUEST_DISASSOCIATE); >>> + >>> PORT_REQUEST_DISASSOCIATE, >>> + false); >>> break; >>> >>> default: >>> @@ -938,6 +947,7 @@ err_exit: >>> * @virtPort: pointer to the object holding port profile parameters >>> * @vmuuid : the UUID of the virtual machine >>> * @vmOp : The VM operation (i.e., create, no-op) >>> + * @setlink_only : Only set the link - dont wait for the link to >>> come up >>> * >>> * Associate a port on a swtich with a profile. This function >>> * may notify a kernel driver or an external daemon to run >>> @@ -954,7 +964,8 @@ virNetDevVPortProfileAssociate(const char >>> *macvtap_ifname, >>> const unsigned char *macvtap_macaddr, >>> const char *linkdev, >>> const unsigned char *vmuuid, >>> - enum virNetDevVPortProfileOp vmOp) >>> + enum virNetDevVPortProfileOp vmOp, >>> + bool setlink_only) >>> { >>> int rc = 0; >>> >>> @@ -976,7 +987,8 @@ virNetDevVPortProfileAssociate(const char >>> *macvtap_ifname, >>> virtPort, >>> (vmOp == >>> VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) >>> ? >>> VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE >>> - : >>> VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE); >>> + : >>> VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE, >>> + setlink_only); >>> break; >>> >>> case VIR_NETDEV_VPORT_PROFILE_8021QBH: >>> @@ -1033,7 +1045,7 @@ virNetDevVPortProfileDisassociate(const char >>> *macvtap_ifname, >>> case VIR_NETDEV_VPORT_PROFILE_8021QBG: >>> rc = virNetDevVPortProfileOp8021Qbg(macvtap_ifname, >>> macvtap_macaddr, >>> virtPort, >>> - >>> VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE); >>> + >>> VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE, false); >>> break; >>> >>> case VIR_NETDEV_VPORT_PROFILE_8021QBH: >>> @@ -1056,7 +1068,8 @@ int virNetDevVPortProfileAssociate(const >>> char *macvtap_ifname ATTRIBUTE_UNUSED, >>> const unsigned char *macvtap_macaddr >>> ATTRIBUTE_UNUSED, >>> const char *linkdev ATTRIBUTE_UNUSED, >>> const unsigned char *vmuuid >>> ATTRIBUTE_UNUSED, >>> - enum virNetDevVPortProfileOp vmOp >>> ATTRIBUTE_UNUSED) >>> + enum virNetDevVPortProfileOp vmOp >>> ATTRIBUTE_UNUSED, >>> + bool setlink_only) >>> { >>> virReportSystemError(ENOSYS, "%s", >>> _("Virtual port profile association not >>> supported on this platform")); >>> diff --git a/src/util/virnetdevvportprofile.h b/src/util/ >>> virnetdevvportprofile.h >>> index 7a8d81f..fe80086 100644 >>> --- a/src/util/virnetdevvportprofile.h >>> +++ b/src/util/virnetdevvportprofile.h >>> @@ -81,7 +81,8 @@ int virNetDevVPortProfileAssociate(const char >>> *ifname, >>> const unsigned char *macaddr, >>> const char *linkdev, >>> const unsigned char *vmuuid, >>> - enum virNetDevVPortProfileOp >>> vmOp) >>> + enum virNetDevVPortProfileOp >>> vmOp, >>> + bool setlink_only) >>> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) >>> ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; >>> >> >> -- >> libvir-list mailing list >> libvir-list@redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list

On 02/24/2012 09:29 AM, D. Herrendoerfer wrote:
The callback mechanism is not re-armed when libvirt is restarted now. The reason is: lldpad remembers who sent the associate by pid - since in theory there could be multiple agents performing associations. So if the libvirt pid changes, there is little we can do now.
This seems very problematic to me - it is assumed that libvirt can be restarted at any time with no ill effects, and restarting libvirt is often suggested as a solution to clearing up problems (e.g. if some other program stomps on libvirt's iptables rules). What can be done to eliminate this problem? Perhaps there needs to be a "re-associate" message - it could be sent each time libvirt restarts for each association it's currently tracking.

On Feb 29, 2012, at 5:36 PM, Laine Stump wrote:
On 02/24/2012 09:29 AM, D. Herrendoerfer wrote:
The callback mechanism is not re-armed when libvirt is restarted now. The reason is: lldpad remembers who sent the associate by pid - since in theory there could be multiple agents performing associations. So if the libvirt pid changes, there is little we can do now.
This seems very problematic to me - it is assumed that libvirt can be restarted at any time with no ill effects, and restarting libvirt is often suggested as a solution to clearing up problems (e.g. if some other program stomps on libvirt's iptables rules).
Yes. That's the way it should be.
What can be done to eliminate this problem? Perhaps there needs to be a "re-associate" message - it could be sent each time libvirt restarts for each association it's currently tracking.
I love the idea. When libvirt starts up. It probes for running VM with vepa associations, sends one associate and re-armes the callback. This would even help situations when the switch infrastructure is out- of-sync with the host system. But how ? Put a special setup function in virnetdevmacvlan.c and call it from domain_conf once all info has been collected and the VM is in running state ? Dirk H
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

While going through this code to clean up the white-space problems, I found 3 issues that need to be addressed before I can push it. Sorry I missed these before. If you can base the new (and I hope final! :-) version on the version where I've already corrected the whitespace, that would be very helpful (otherwise I'll need to merge back with my updated version, a mechanical, but time consuming process). Rather than post them to the mailing list, I've pushed my updated versions of your patches to the "lldpad2" branch of my staging repo on gitorious: git://gitorious.org/~laine/libvirt/laine-staging.git You should be able to get that branch into your local repo like this: git remote add laine-staging \ git://gitorious.org/~laine/libvirt/laine-staging.git git fetch laine-staging git checkout laine-staging/llpad2 You can then cherry-pick those commits into your own branch, or create a local branch based on it, then just squash in changes addressing the three problems I point out below. Once these are fixed, I will ACK and push it. Thanks! On 02/22/2012 08:17 AM, D. Herrendoerfer wrote:
From: "D. Herrendoerfer" <d.herrendoerfer@herrendoerfer.name>
Add de-association handling for 802.1qbg (vepa) via lldpad netlink messages. Also adds the possibility to perform an association request without waiting for a confirmation.
Signed-off-by: D. Herrendoerfer <d.herrendoerfer@herrendoerfer.name> --- src/qemu/qemu_migration.c | 2 +- src/util/virnetdevmacvlan.c | 315 +++++++++++++++++++++++++++++++++++++- src/util/virnetdevvportprofile.c | 33 +++-- src/util/virnetdevvportprofile.h | 3 +- 4 files changed, 339 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 12cfbde..31428f8 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2567,7 +2567,7 @@ qemuMigrationVPAssociatePortProfiles(virDomainDefPtr def) { net->mac, virDomainNetGetActualDirectDev(net), def->uuid, - VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH) < 0) + VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH, false) < 0) goto err_exit; } last_good_net = i; diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 25d0846..b3e3325 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 @@ -68,6 +69,8 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, VIR_NETDEV_MACVLAN_MODE_LAST, # include "virfile.h" # include "virnetlink.h" # include "virnetdev.h" +# include "virpidfile.h" +
# define MACVTAP_NAME_PREFIX "macvtap" # define MACVTAP_NAME_PATTERN "macvtap%d" @@ -75,6 +78,7 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, VIR_NETDEV_MACVLAN_MODE_LAST, # define MACVLAN_NAME_PREFIX "macvlan" # define MACVLAN_NAME_PATTERN "macvlan%d"
+ /** * virNetDevMacVLanCreate: * @@ -445,6 +449,293 @@ static const uint32_t modeMap[VIR_NETDEV_MACVLAN_MODE_LAST] = { [VIR_NETDEV_MACVLAN_MODE_PASSTHRU] = MACVLAN_MODE_PASSTHRU, };
+/* Struct to hold the state and configuration of a 802.1qbg port */ +struct virNetlinkCallbackData { + char cr_ifname[64];
Issue 1, part 1: This should either be IFNAMSIZ bytes long, or be a char* (pointing to separately allocated data) instead of a fixed size. This string is copied into strings that are sent to the kernel for interface names, and interface names have a maximum size of IFNAMSIZE (16); if you need consistency of the name in all places (even in log messages), then you should limit it at the source, but otherwise there's no sense in checking string sizes both at this level and then again at the lower level. My preference would be a char*, I think - leave it to the lower levels to decide if it needs to be chopped.
+ virNetDevVPortProfilePtr virtPortProfile; + const unsigned char *macaddress; + const char *linkdev; + const unsigned char *vmuuid; + enum virNetDevVPortProfileOp vmOp; + unsigned int linkState; +}; + +typedef struct virNetlinkCallbackData *virNetlinkCallbackDataPtr; + +#define INSTANCE_STRLEN 36 + +static int instance2str(const unsigned char *p, char *dst, size_t size) +{ + if (dst && size > INSTANCE_STRLEN) { + snprintf(dst, size, "%02x%02x%02x%02x-%02x%02x-%02x%02x-" + "%02x%02x-%02x%02x%02x%02x%02x%02x", + p[0], p[1], p[2], p[3], + p[4], p[5], p[6], p[7], + p[8], p[9], p[10], p[11], p[12], p[13], p[14], p[15]); + return 0; + } + return -1; +} + +# define LLDPAD_PID_FILE "/var/run/lldpad.pid" +# define VIRIP_PID_FILE "/var/run/virip.pid" + +/** + * 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, + bool *handled, + void *opaque) +{ + struct nla_policy ifla_vf_policy[IFLA_VF_MAX + 1] = { + [IFLA_VF_MAC] = {.minlen = sizeof(struct ifla_vf_mac), + .maxlen = sizeof(struct ifla_vf_mac)}, + [IFLA_VF_VLAN] = {.minlen = sizeof(struct ifla_vf_vlan), + .maxlen = sizeof(struct ifla_vf_vlan)}, + }; + + struct nla_policy ifla_port_policy[IFLA_PORT_MAX + 1] = { + [IFLA_PORT_RESPONSE] = {.type = NLA_U16}, + }; + + struct nlattr *tb[IFLA_MAX + 1], + *tb3[IFLA_PORT_MAX + 1], + *tb_vfinfo[IFLA_VF_MAX + 1], *tb_vfinfo_list; + + struct ifinfomsg ifinfo; + struct nlmsghdr *hdr; + void *data; + int rem; + char *ifname; + bool indicate = false; + virNetlinkCallbackDataPtr calld = opaque; + pid_t lldpad_pid = 0; + pid_t virip_pid = 0; + + hdr = (struct nlmsghdr *) msg; + data = nlmsg_data(hdr); + + /* Quickly decide if we want this or not */ + + if (virPidFileReadPath(LLDPAD_PID_FILE, &lldpad_pid) < 0) + return; + + ignore_value(virPidFileReadPath(VIRIP_PID_FILE, &virip_pid)); + + if (hdr->nlmsg_pid != lldpad_pid && hdr->nlmsg_pid != virip_pid) + return; // we only care for lldpad and virip messages + if (hdr->nlmsg_type != RTM_SETLINK) + return; // we only care for RTM_SETLINK + if (*handled) + return; // if it has been handeled - dont handle again + + /* DEBUG start */ + VIR_INFO("netlink message nl_sockaddr: %p len: %d", peer, length); + VIR_DEBUG("nlmsg_type = 0x%02x",hdr->nlmsg_type); + VIR_DEBUG("nlmsg_len = 0x%04x",hdr->nlmsg_len ); + VIR_DEBUG("nlmsg_pid = %d",hdr->nlmsg_pid ); + VIR_DEBUG("nlmsg_seq = 0x%08x",hdr->nlmsg_seq ); + VIR_DEBUG("nlmsg_flags = 0x%04x",hdr->nlmsg_flags ); + + VIR_DEBUG("lldpad pid = %d",lldpad_pid); + + switch (hdr->nlmsg_type) { + case RTM_NEWLINK: + case RTM_DELLINK: + case RTM_SETLINK: + case RTM_GETLINK: + VIR_DEBUG(" IFINFOMSG\n"); + VIR_DEBUG(" ifi_family = 0x%02x\n", + ((struct ifinfomsg *)data)->ifi_family); + VIR_DEBUG(" ifi_type = 0x%x\n", + ((struct ifinfomsg *)data)->ifi_type); + VIR_DEBUG(" ifi_index = %i\n", + ((struct ifinfomsg *)data)->ifi_index); + VIR_DEBUG(" ifi_flags = 0x%04x\n", + ((struct ifinfomsg *)data)->ifi_flags); + VIR_DEBUG(" ifi_change = 0x%04x\n", + ((struct ifinfomsg *)data)->ifi_change); + } + /* DEBUG end */ + + /* Parse netlink message assume a setlink with vfports */ + memcpy(&ifinfo, NLMSG_DATA(hdr), sizeof ifinfo); + VIR_DEBUG("family:%#x type:%#x index:%d flags:%#x change:%#x", + ifinfo.ifi_family, ifinfo.ifi_type, ifinfo.ifi_index, + ifinfo.ifi_flags, ifinfo.ifi_change); + if (nlmsg_parse(hdr, sizeof ifinfo, + (struct nlattr **)&tb, IFLA_MAX, NULL)) { + VIR_DEBUG("error parsing request..."); + return; + } + + if (tb[IFLA_VFINFO_LIST]) { + VIR_DEBUG("FOUND IFLA_VFINFO_LIST!"); + + nla_for_each_nested(tb_vfinfo_list, tb[IFLA_VFINFO_LIST], rem) { + if (nla_type(tb_vfinfo_list) != IFLA_VF_INFO) { + VIR_DEBUG("nested parsing of" + "IFLA_VFINFO_LIST failed."); + return; + } + if (nla_parse_nested(tb_vfinfo, IFLA_VF_MAX, + tb_vfinfo_list, ifla_vf_policy)) { + VIR_DEBUG("nested parsing of " + "IFLA_VF_INFO failed."); + return; + } + } + + if (tb_vfinfo[IFLA_VF_MAC]) { + struct ifla_vf_mac *mac = + RTA_DATA(tb_vfinfo[IFLA_VF_MAC]); + unsigned char *m = mac->mac; + + VIR_DEBUG("IFLA_VF_MAC=%2x:%2x:%2x:%2x:%2x:%2x", + m[0], m[1], m[2], m[3], m[4], m[5]); + + if (memcmp(calld->macaddress, m, VIR_MAC_BUFLEN)) + { + /* Repeat the same check for a broadcast mac */ + unsigned char broadcastmac[VIR_MAC_BUFLEN]; + int i; + + for (i=0;i<VIR_MAC_BUFLEN;i++) + broadcastmac[i]=0xFF; + + if (memcmp(calld->macaddress, broadcastmac, VIR_MAC_BUFLEN)) { + VIR_DEBUG("MAC address match failed."); + return; + }
Issue 2: Instead of using the loop to create a complete broadcast mac address, then doing memcmp to compare to the current mac address, just change the loop to this: for (i = 0; i < VIR_MAC_BUFLEN; i++) { if (calld->macaddress[i] != 0xff) { VIR_DEBUG("MAC address match failed (wasn't broadcast)"); return; } } and get rid of the memcmp.
+ } + } + + if (tb_vfinfo[IFLA_VF_VLAN]) { + struct ifla_vf_vlan *vlan = + RTA_DATA(tb_vfinfo[IFLA_VF_VLAN]); + + VIR_DEBUG("IFLA_VF_VLAN=%d", vlan->vlan); + } + } + + if (tb[IFLA_IFNAME]) { + ifname = (char *)RTA_DATA(tb[IFLA_IFNAME]); + VIR_DEBUG("IFLA_IFNAME=%s\n", ifname); + } + + if (tb[IFLA_OPERSTATE]) { + rem = *(unsigned short *)RTA_DATA(tb[IFLA_OPERSTATE]); + VIR_DEBUG("IFLA_OPERSTATE=%d\n", rem); + } + + if (tb[IFLA_VF_PORTS]) { + struct nlattr *tb_vf_ports; + + VIR_DEBUG("found IFLA_VF_PORTS\n"); + nla_for_each_nested(tb_vf_ports, tb[IFLA_VF_PORTS], rem) { + + VIR_DEBUG("iterating\n"); + if (nla_type(tb_vf_ports) != IFLA_VF_PORT) { + VIR_DEBUG("not a IFLA_VF_PORT. skipping\n"); + continue; + } + if (nla_parse_nested(tb3, IFLA_PORT_MAX, tb_vf_ports, + ifla_port_policy)) { + VIR_DEBUG("nested parsing on level 2" + " failed."); + } + if (tb3[IFLA_PORT_VF]) { + VIR_DEBUG("IFLA_PORT_VF=%d", + *(uint32_t + *) (RTA_DATA(tb3[IFLA_PORT_VF]))); + } + if (tb3[IFLA_PORT_PROFILE]) { + VIR_DEBUG("IFLA_PORT_PROFILE=%s", (char *) + RTA_DATA(tb3[IFLA_PORT_PROFILE])); + } + + if (tb3[IFLA_PORT_VSI_TYPE]) { + struct ifla_port_vsi *pvsi; + int tid = 0; + + pvsi = (struct ifla_port_vsi *) + RTA_DATA(tb3[IFLA_PORT_VSI_TYPE]); + tid = pvsi->vsi_type_id[2] << 16 | + pvsi->vsi_type_id[1] << 8 | + pvsi->vsi_type_id[0]; + + VIR_DEBUG("mgr_id: %d", pvsi->vsi_mgr_id); + VIR_DEBUG("type_id: %d", tid); + VIR_DEBUG("type_version: %d", + pvsi->vsi_type_version); + } + + if (tb3[IFLA_PORT_INSTANCE_UUID]) { + char instance[INSTANCE_STRLEN + 2]; + unsigned char *uuid; + + uuid = (unsigned char *) + RTA_DATA(tb3[IFLA_PORT_INSTANCE_UUID]); + instance2str(uuid, instance, sizeof(instance)); + VIR_DEBUG("IFLA_PORT_INSTANCE_UUID=%s\n", + instance); + } + + if (tb3[IFLA_PORT_REQUEST]) { + uint8_t req = *(uint8_t *) RTA_DATA(tb3[IFLA_PORT_REQUEST]); + VIR_DEBUG("IFLA_PORT_REQUEST=%d", req); + + if ( req == PORT_REQUEST_DISASSOCIATE ) { + VIR_DEBUG("Set dissaccociated."); + indicate=true; + } + } + + if (tb3[IFLA_PORT_RESPONSE]) { + VIR_DEBUG("IFLA_PORT_RESPONSE=%d\n", *(uint16_t *) + RTA_DATA(tb3[IFLA_PORT_RESPONSE])); + } + } + } + + if (!indicate) { + return; + } + + VIR_INFO("Re-send 802.1qbg associate request:"); + VIR_INFO(" if: %s",calld->cr_ifname ); + VIR_INFO(" lf: %s",calld->linkdev ); + VIR_INFO(" mac: %02x:%02x:%02x:%02x:%02x:%02x",calld->macaddress[0], + calld->macaddress[1], + calld->macaddress[2], + calld->macaddress[3], + calld->macaddress[4], + calld->macaddress[5] ); + + ignore_value(virNetDevVPortProfileAssociate(calld->cr_ifname, + calld->virtPortProfile, + calld->macaddress, + calld->linkdev, + calld->vmuuid, calld->vmOp, true)); + + *handled = true; + return; +} + /** * virNetDevMacVLanCreateWithVPortProfile: * Create an instance of a macvtap device and open its tap character @@ -547,7 +838,7 @@ create_name: virtPortProfile, macaddress, linkdev, - vmuuid, vmOp) < 0) { + vmuuid, vmOp, false) < 0) { rc = -1; goto link_del_exit; } @@ -589,6 +880,23 @@ create_name: goto disassociate_exit; }
+ if (virNetlinkEventServiceIsRunning()) { + virNetlinkCallbackDataPtr calld; + + if (VIR_ALLOC(calld) < 0) { + virReportOOMError(); + goto disassociate_exit; + } + + strncpy(calld->cr_ifname,cr_ifname,64);
Issue 1, Part 2: strncpy isn't allowed in libvirt code. Instead, you should either use virStrncpy (which is safer, as it always makes sure the string is null-terminated) or just change cr_ifname to be char* (as suggested above), and fill it in with strdup() (which is allowed, but be sure to check for failure!)
+ calld->virtPortProfile=virtPortProfile; + calld->macaddress=macaddress; + calld->linkdev=linkdev; + calld->vmuuid=vmuuid; + calld->vmOp=vmOp; + + virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback, calld, macaddress); + }
Issue 3, Part 1: Note that at this point, "calld" is owned by the netlink event handler - you haven't retained any other pointer to that memory. This will become an issue when you later decide to remove the event...
return rc;
@@ -638,6 +946,9 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, if (virNetDevMacVLanDelete(ifname) < 0) ret = -1; } + + virNetlinkEventRemoveClient(0,macaddr); +
Issue 3, part 2: ... you've now removed the event, but the calld associated with it hasn't been freed. Since the client code doesn't retain a pointer to it, I think the netlink event code needs a new bit added to the API - virNetlinkEventRemoveClient needs to accept a pointer to a destructor function: typedef void (*virNetlinkEventRemoveCallback)(int watch, const unsigned char *macaddr, void *opaque); int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr, virNetlinkEventRemoveCallback cb); If cb is non-NULL, it should be called with pointers to the appropriate data. In the case of this particular client, the callback should look like this: static void virNetDevMacVLanVPortProfileDestroyCallback(int watch ATTRIBUTE_UNUSED, unsigned char *macaddr ATTRIBUTE_UNUSED, void *opaque) { virNetlinkCallbackDataPtr calld = opaque; /* the part inside the if() assumes you changed cr_ifname to char * */ if (calld) { VIR_FREE(calld->cr_ifname); } VIR_FREE(cr_ifname); } and when you remove the client: virNetlinkEventRemoveClient, 0, macaddr, virNetDevMacVLanVPortProfileDestroyCallback); This will eliminate your memory leak (and get the ACK so we can push both patches! :-)
return ret; }
diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index faadc3a..554f128 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -650,7 +650,8 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex, const unsigned char *instanceId, const unsigned char *hostUUID, int32_t vf, - uint8_t op) + uint8_t op, + bool setlink_only) { int rc; unsigned char *recvbuf = NULL; @@ -675,6 +676,9 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex, return rc; }
+ if (setlink_only) /*for re-associations on existing links*/ + return 0; + while (--repeats >= 0) { rc = virNetDevVPortProfileLinkDump(NULL, ifindex, nltarget_kernel, tb, &recvbuf, virNetDevVPortProfileGetLldpadPid); @@ -751,7 +755,8 @@ static int virNetDevVPortProfileOp8021Qbg(const char *ifname, const unsigned char *macaddr, const virNetDevVPortProfilePtr virtPort, - enum virNetDevVPortProfileLinkOp virtPortOp) + enum virNetDevVPortProfileLinkOp virtPortOp, + bool setlink_only) { int rc = 0; int op = PORT_REQUEST_ASSOCIATE; @@ -804,7 +809,8 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname, virtPort->u.virtPort8021Qbg.instanceID, NULL, vf, - op); + op, + setlink_only);
err_exit:
@@ -892,7 +898,8 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname, (virtPortOp == VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE_RR) ? PORT_REQUEST_PREASSOCIATE_RR - : PORT_REQUEST_ASSOCIATE); + : PORT_REQUEST_ASSOCIATE, + false); if (rc == -2) /* Association timed out, disassociate */ virNetDevVPortProfileOpCommon(NULL, ifindex, @@ -904,7 +911,8 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname, NULL, NULL, vf, - PORT_REQUEST_DISASSOCIATE); + PORT_REQUEST_DISASSOCIATE, + false); break;
case VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE: @@ -917,7 +925,8 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname, NULL, NULL, vf, - PORT_REQUEST_DISASSOCIATE); + PORT_REQUEST_DISASSOCIATE, + false); break;
default: @@ -938,6 +947,7 @@ err_exit: * @virtPort: pointer to the object holding port profile parameters * @vmuuid : the UUID of the virtual machine * @vmOp : The VM operation (i.e., create, no-op) + * @setlink_only : Only set the link - dont wait for the link to come up * * Associate a port on a swtich with a profile. This function * may notify a kernel driver or an external daemon to run @@ -954,7 +964,8 @@ virNetDevVPortProfileAssociate(const char *macvtap_ifname, const unsigned char *macvtap_macaddr, const char *linkdev, const unsigned char *vmuuid, - enum virNetDevVPortProfileOp vmOp) + enum virNetDevVPortProfileOp vmOp, + bool setlink_only) { int rc = 0;
@@ -976,7 +987,8 @@ virNetDevVPortProfileAssociate(const char *macvtap_ifname, virtPort, (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) ? VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE - : VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE); + : VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE, + setlink_only); break;
case VIR_NETDEV_VPORT_PROFILE_8021QBH: @@ -1033,7 +1045,7 @@ virNetDevVPortProfileDisassociate(const char *macvtap_ifname, case VIR_NETDEV_VPORT_PROFILE_8021QBG: rc = virNetDevVPortProfileOp8021Qbg(macvtap_ifname, macvtap_macaddr, virtPort, - VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE); + VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE, false); break;
case VIR_NETDEV_VPORT_PROFILE_8021QBH: @@ -1056,7 +1068,8 @@ int virNetDevVPortProfileAssociate(const char *macvtap_ifname ATTRIBUTE_UNUSED, const unsigned char *macvtap_macaddr ATTRIBUTE_UNUSED, const char *linkdev ATTRIBUTE_UNUSED, const unsigned char *vmuuid ATTRIBUTE_UNUSED, - enum virNetDevVPortProfileOp vmOp ATTRIBUTE_UNUSED) + enum virNetDevVPortProfileOp vmOp ATTRIBUTE_UNUSED, + bool setlink_only) { virReportSystemError(ENOSYS, "%s", _("Virtual port profile association not supported on this platform")); diff --git a/src/util/virnetdevvportprofile.h b/src/util/virnetdevvportprofile.h index 7a8d81f..fe80086 100644 --- a/src/util/virnetdevvportprofile.h +++ b/src/util/virnetdevvportprofile.h @@ -81,7 +81,8 @@ int virNetDevVPortProfileAssociate(const char *ifname, const unsigned char *macaddr, const char *linkdev, const unsigned char *vmuuid, - enum virNetDevVPortProfileOp vmOp) + enum virNetDevVPortProfileOp vmOp, + bool setlink_only) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK;

From: "D. Herrendoerfer" <d.herrendoerfer@herrendoerfer.name> this patch adds the changes proposed by Laine Stump to netlink event code. Signed-off-by: D. Herrendoerfer <d.herrendoerfer@herrendoerfer.name> --- src/util/virnetdevmacvlan.c | 47 +++++++++++++++++++++++++++++++++--------- src/util/virnetlink.c | 13 ++++++++++- src/util/virnetlink.h | 7 ++++- 3 files changed, 53 insertions(+), 14 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 4256349..155ce79 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -448,7 +448,7 @@ static const uint32_t modeMap[VIR_NETDEV_MACVLAN_MODE_LAST] = { /* Struct to hold the state and configuration of a 802.1qbg port */ struct virNetlinkCallbackData { - char cr_ifname[64]; + char *cr_ifname; virNetDevVPortProfilePtr virtPortProfile; const unsigned char *macaddress; const char *linkdev; @@ -606,15 +606,13 @@ virNetDevMacVLanVPortProfileCallback(unsigned char *msg, if (memcmp(calld->macaddress, m, VIR_MAC_BUFLEN)) { /* Repeat the same check for a broadcast mac */ - unsigned char broadcastmac[VIR_MAC_BUFLEN]; int i; - for (i = 0;i < VIR_MAC_BUFLEN; i++) - broadcastmac[i] = 0xFF; - - if (memcmp(calld->macaddress, broadcastmac, VIR_MAC_BUFLEN)) { - VIR_DEBUG("MAC address match failed."); - return; + for (i = 0;i < VIR_MAC_BUFLEN; i++) { + if (calld->macaddress[i] != 0xff) { + VIR_DEBUG("MAC address match failed (wasn't broadcast)"); + return; + } } } } @@ -728,6 +726,30 @@ virNetDevMacVLanVPortProfileCallback(unsigned char *msg, } /** + * virNetDevMacVLanVPortProfileDestroyCallback: + * + * @watch: watch whose handle to remove + * @macaddr: macaddr whose handle to remove + * @opaque: Contains vital information regarding the associated vm + * + * This function is called when a netlink message handler is terminated. + * The function frees locally allocated data referenced in the opaque + * data. + */ +static void +virNetDevMacVLanVPortProfileDestroyCallback(int watch ATTRIBUTE_UNUSED, + const unsigned char *macaddr ATTRIBUTE_UNUSED, + void *opaque) +{ + virNetlinkCallbackDataPtr calld = opaque; + + if (calld) { + VIR_FREE(calld->cr_ifname); + } +} + + +/** * virNetDevMacVLanCreateWithVPortProfile: * Create an instance of a macvtap device and open its tap character * device. @@ -879,7 +901,12 @@ create_name: goto disassociate_exit; } - strncpy(calld->cr_ifname, cr_ifname, 64); + calld->cr_ifname=strdup(cr_ifname); + if (calld->cr_ifname == NULL) { + virReportOOMError(); + goto disassociate_exit; + } + calld->virtPortProfile = virtPortProfile; calld->macaddress = macaddress; calld->linkdev = linkdev; @@ -938,7 +965,7 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, ret = -1; } - virNetlinkEventRemoveClient(0, macaddr); + virNetlinkEventRemoveClient(0, macaddr, virNetDevMacVLanVPortProfileDestroyCallback); return ret; } diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 751aa67..de6a135 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -446,6 +446,7 @@ error: * * @watch: watch whose handle to remove * @macaddr: macaddr whose handle to remove + * &cb: callback for the destruction of local data * * Unregister a callback from a netlink monitor. * The handler function referenced will no longer receive netlink messages. @@ -454,7 +455,8 @@ error: * returns -1 if the file handle was not registered, 0 upon success */ int -virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr) { +virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr, + virNetlinkEventRemoveCallback cb) { int i; int ret = -1; virNetlinkEventSrvPrivatePtr srv = server; @@ -474,6 +476,9 @@ virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr) { continue; if (watch && srv->handles[i].watch == watch) { + void *cpopaque = srv->handles[i].opaque; + (cb)( watch, macaddr, cpopaque); + VIR_FREE(srv->handles[i].opaque); srv->handles[i].deleted = VIR_NETLINK_HANDLE_DELETED; VIR_DEBUG("removed client: %d by index.", @@ -482,6 +487,9 @@ virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr) { goto error; } if (!watch && memcmp(macaddr, srv->handles[i].macaddr, VIR_MAC_BUFLEN) == 0) { + void *cpopaque = srv->handles[i].opaque; + (cb)( watch, macaddr, cpopaque); + VIR_FREE(srv->handles[i].opaque); srv->handles[i].deleted = VIR_NETLINK_HANDLE_DELETED; VIR_DEBUG("removed client: %d by mac.", @@ -567,7 +575,8 @@ int virNetlinkEventAddClient(virNetlinkEventHandleCallback cb, void *opaque, /** * virNetlinkEventRemoveClient: unregister a callback from a netlink monitor */ -int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr) { +int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr, + virNetlinkEventRemoveCallback cb) { netlinkError(VIR_ERR_INTERNAL_ERROR, "%s", # if defined(__linux__) && !defined(HAVE_LIBNL) diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 1365afa..b787a8f 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -38,7 +38,9 @@ int virNetlinkCommand(struct nl_msg *nl_msg, unsigned char **respbuf, unsigned int *respbuflen, int nl_pid); -typedef void (*virNetlinkEventHandleCallback)( unsigned char *msg, int length, struct sockaddr_nl *peer, bool *handled, void *opaque); +typedef void (*virNetlinkEventHandleCallback)(unsigned char *msg, int length, struct sockaddr_nl *peer, bool *handled, void *opaque); + +typedef void (*virNetlinkEventRemoveCallback)(int watch, const unsigned char *macaddr, void *opaque); /** * stopNetlinkEventServer: stop the monitor to receive netlink messages for libvirtd @@ -63,6 +65,7 @@ int virNetlinkEventAddClient(virNetlinkEventHandleCallback cb, void *opaque, con /** * virNetlinkEventRemoveClient: unregister a callback from a netlink monitor */ -int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr); +int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr, + virNetlinkEventRemoveCallback cb); #endif /* __VIR_NETLINK_H__ */ -- 1.7.7.6

Since I found a couple other problems, but have made you suffer through enough back and forth already, I've made some final suggested changes myself, and am sending a diff patch as a response to this message (differences from your latest version to mine), as well as doing a repost of the original two patches with your latest changes and mine squashed in. Please 1) take a look at the changes in my diff patches , then 2) test the updated versions of the full patches (I'm unable to test beyond compiling), and send your ACK if they're okay. Once I have your ACK back, I'll push (I promise, I won't find any new issues *this* time :-) On 02/28/2012 10:34 AM, D. Herrendoerfer wrote:
From: "D. Herrendoerfer" <d.herrendoerfer@herrendoerfer.name>
this patch adds the changes proposed by Laine Stump to netlink event code.
Signed-off-by: D. Herrendoerfer <d.herrendoerfer@herrendoerfer.name> --- src/util/virnetdevmacvlan.c | 47 +++++++++++++++++++++++++++++++++--------- src/util/virnetlink.c | 13 ++++++++++- src/util/virnetlink.h | 7 ++++- 3 files changed, 53 insertions(+), 14 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 4256349..155ce79 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -448,7 +448,7 @@ static const uint32_t modeMap[VIR_NETDEV_MACVLAN_MODE_LAST] = {
/* Struct to hold the state and configuration of a 802.1qbg port */ struct virNetlinkCallbackData { - char cr_ifname[64]; + char *cr_ifname; virNetDevVPortProfilePtr virtPortProfile; const unsigned char *macaddress; const char *linkdev; @@ -606,15 +606,13 @@ virNetDevMacVLanVPortProfileCallback(unsigned char *msg, if (memcmp(calld->macaddress, m, VIR_MAC_BUFLEN)) { /* Repeat the same check for a broadcast mac */ - unsigned char broadcastmac[VIR_MAC_BUFLEN]; int i;
- for (i = 0;i < VIR_MAC_BUFLEN; i++) - broadcastmac[i] = 0xFF; - - if (memcmp(calld->macaddress, broadcastmac, VIR_MAC_BUFLEN)) { - VIR_DEBUG("MAC address match failed."); - return; + for (i = 0;i < VIR_MAC_BUFLEN; i++) { + if (calld->macaddress[i] != 0xff) { + VIR_DEBUG("MAC address match failed (wasn't broadcast)"); + return; + } } } } @@ -728,6 +726,30 @@ virNetDevMacVLanVPortProfileCallback(unsigned char *msg, }
/** + * virNetDevMacVLanVPortProfileDestroyCallback: + * + * @watch: watch whose handle to remove + * @macaddr: macaddr whose handle to remove + * @opaque: Contains vital information regarding the associated vm + * + * This function is called when a netlink message handler is terminated. + * The function frees locally allocated data referenced in the opaque + * data. + */ +static void +virNetDevMacVLanVPortProfileDestroyCallback(int watch ATTRIBUTE_UNUSED, + const unsigned char *macaddr ATTRIBUTE_UNUSED, + void *opaque) +{ + virNetlinkCallbackDataPtr calld = opaque; + + if (calld) { + VIR_FREE(calld->cr_ifname); + } +}
You also need to do "VIR_FREE(calld) here, right?
+ + +/** * virNetDevMacVLanCreateWithVPortProfile: * Create an instance of a macvtap device and open its tap character * device. @@ -879,7 +901,12 @@ create_name: goto disassociate_exit; }
- strncpy(calld->cr_ifname, cr_ifname, 64); + calld->cr_ifname=strdup(cr_ifname); + if (calld->cr_ifname == NULL) { + virReportOOMError(); + goto disassociate_exit; + } + calld->virtPortProfile = virtPortProfile; calld->macaddress = macaddress; calld->linkdev = linkdev; @@ -938,7 +965,7 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, ret = -1; }
- virNetlinkEventRemoveClient(0, macaddr); + virNetlinkEventRemoveClient(0, macaddr, virNetDevMacVLanVPortProfileDestroyCallback);
return ret; } diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 751aa67..de6a135 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -446,6 +446,7 @@ error: * * @watch: watch whose handle to remove * @macaddr: macaddr whose handle to remove + * &cb: callback for the destruction of local data * * Unregister a callback from a netlink monitor. * The handler function referenced will no longer receive netlink messages. @@ -454,7 +455,8 @@ error: * returns -1 if the file handle was not registered, 0 upon success */ int -virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr) { +virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr, + virNetlinkEventRemoveCallback cb) {
Once I actually looked at my suggestion in practice, I realized that, in order for virNetlinkEventServiceStart to be able to properly remove clients (and thus avoid memory leaks), the remove callback needs to be given when *adding* the client, and stored in the client data. I made that modification, and will be sending a diff patch, as well as updated final versions of both original patches.
int i; int ret = -1; virNetlinkEventSrvPrivatePtr srv = server; @@ -474,6 +476,9 @@ virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr) { continue;
if (watch && srv->handles[i].watch == watch) { + void *cpopaque = srv->handles[i].opaque; + (cb)( watch, macaddr, cpopaque);
This should only be called if non-NULL.
+ VIR_FREE(srv->handles[i].opaque);
Ah, I missed that line before. Having this in the server code makes it not exactly opaque - you're requiring that it point to data that is dynamically allocated. I think it would be better to leave this free up to the callback (possibly a client will want to point to static data, or maybe use it to store a handle rather than an actual pointer).
srv->handles[i].deleted = VIR_NETLINK_HANDLE_DELETED; VIR_DEBUG("removed client: %d by index.", @@ -482,6 +487,9 @@ virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr) { goto error; } if (!watch && memcmp(macaddr, srv->handles[i].macaddr, VIR_MAC_BUFLEN) == 0) { + void *cpopaque = srv->handles[i].opaque; + (cb)( watch, macaddr, cpopaque); + VIR_FREE(srv->handles[i].opaque); srv->handles[i].deleted = VIR_NETLINK_HANDLE_DELETED; VIR_DEBUG("removed client: %d by mac.",
Also, I missed all this duplicated code - it's easier to maintain if you combine the two if conditions and just do the body of the loop once (the only difference is the last word of the debug message, and that can easily be handled.
@@ -567,7 +575,8 @@ int virNetlinkEventAddClient(virNetlinkEventHandleCallback cb, void *opaque, /** * virNetlinkEventRemoveClient: unregister a callback from a netlink monitor */ -int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr) { +int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr, + virNetlinkEventRemoveCallback cb) { netlinkError(VIR_ERR_INTERNAL_ERROR, "%s", # if defined(__linux__) && !defined(HAVE_LIBNL) diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 1365afa..b787a8f 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -38,7 +38,9 @@ int virNetlinkCommand(struct nl_msg *nl_msg, unsigned char **respbuf, unsigned int *respbuflen, int nl_pid);
-typedef void (*virNetlinkEventHandleCallback)( unsigned char *msg, int length, struct sockaddr_nl *peer, bool *handled, void *opaque); +typedef void (*virNetlinkEventHandleCallback)(unsigned char *msg, int length, struct sockaddr_nl *peer, bool *handled, void *opaque); + +typedef void (*virNetlinkEventRemoveCallback)(int watch, const unsigned char *macaddr, void *opaque);
/** * stopNetlinkEventServer: stop the monitor to receive netlink messages for libvirtd @@ -63,6 +65,7 @@ int virNetlinkEventAddClient(virNetlinkEventHandleCallback cb, void *opaque, con /** * virNetlinkEventRemoveClient: unregister a callback from a netlink monitor */ -int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr); +int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr, + virNetlinkEventRemoveCallback cb);
#endif /* __VIR_NETLINK_H__ */

Squashed into "util: Add netlink event handling to virnetlink.c" (Note that I am also re-sending the complete patchset with these changes already squashed in). * The remove callback is now an argument of virNetlinnkEventAddClient instead of virNetlinkeRemoveClient, and is stored in the virNetlinkEventHandle. This way the event service shutdown can properly remove any remaining clients to avoid memory leaks. * Several small whitespace changes (moving the opening { of functions down to a separate line, for example). * As said above, the service stop function now removes any remaining clients on the list. * The libvirt convention to check for a failure return from a function is (ret < 0) rather than (ret == -1). That was changed in two places. * If macaddr isn't given when adding a client, it is initialized to all 0's (on a clean init, it would be 0 anyway, but if the entry is being re-used, it would have stale data from the previous use). * In virNetlinkEventRemoveClient, the to if() clauses inside the for loop had identical bodies (except for one word in the debug message). These have been combined into a single if() clause. * Again in virNetlinkEventRemoveClient, the opaque object was being freed by the event service code, which is supposed to treat opaque as a complete unknown. This is no longer done - if opaque is dynamically allocated data that needs to be freed, this should be done in the client-supplied removal callback (as noted earlier, the client has been appropriately changed). Changes squashed into "Add de-association handling to macvlan code" * In the virNetCallbackData struct, the pointers to data that are in the domain's config object are a bit dangerous - it's possible that the domain's config could be updated while the domain remains active, freeing the original config object and allocating a new one. This would leave the virNetlinkCallbackData pointing at garbage data. To fix this, the struct has had all consts removed, the object is initialized by allocating new memory for each of these items and copying (rather than just pointing at the original), and the remove callback frees all of the items. The result is that the callback data object is completely decoupled from the config object. * Since the callback data object needs to be freed both in the remove callback as well as in the error path at the time the client is added, a separate virNetlinkCallbackDataFree() function has been added. The remove callback now simply calls that function. * Previously the callback data object itself was freed by the netlink event service code as the client was being removed. This makes it impossible to use the supposedly "opaque" data as anything other than a pure pointer to dynamically allocated data (a client might want to use it simply as a cookie, or pointer to a piece of static data). Now this object is freed by the client's remove callback function instead. * The error path of virNetDevMacVLanCreateWithVPortProfile had been missing a call to free the callback data object if the strdup(cr_ifname) failed. Now with even more error cases (since all the other fields are also dynamically allocated), it became even more apparent that disassociate_exit: needed to do this. Since all of these cases need to call virReportOOMError(), I factored that out into a new goto label "memory_error". * Due to a change in the netlink event service code, the remove callback function is now given when calling virNetlinkEventAddClient rather than when calling virNetlinkEventRemoveClient. --- src/util/virnetdevmacvlan.c | 79 +++++++++++------ src/util/virnetlink.c | 197 ++++++++++++++++++++++++------------------- src/util/virnetlink.h | 7 +- 3 files changed, 165 insertions(+), 118 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 1c4d082..d36b326 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -450,9 +450,9 @@ static const uint32_t modeMap[VIR_NETDEV_MACVLAN_MODE_LAST] = { struct virNetlinkCallbackData { char *cr_ifname; virNetDevVPortProfilePtr virtPortProfile; - const unsigned char *macaddress; - const char *linkdev; - const unsigned char *vmuuid; + unsigned char *macaddress; + char *linkdev; + unsigned char *vmuuid; enum virNetDevVPortProfileOp vmOp; unsigned int linkState; }; @@ -726,6 +726,29 @@ virNetDevMacVLanVPortProfileCallback(unsigned char *msg, } /** + * virNetlinkCallbackDataFree + * + * @calld: pointer to a virNetlinkCallbackData object to free + * + * This function frees all the data associated with a virNetlinkCallbackData object + * as well as the object itself. If called with NULL, it does nothing. + * + * Returns nothing. + */ +static void +virNetlinkCallbackDataFree(virNetlinkCallbackDataPtr calld) +{ + if (calld) { + VIR_FREE(calld->cr_ifname); + VIR_FREE(calld->virtPortProfile); + VIR_FREE(calld->macaddress); + VIR_FREE(calld->linkdev); + VIR_FREE(calld->vmuuid); + } + VIR_FREE(calld); +} + +/** * virNetDevMacVLanVPortProfileDestroyCallback: * * @watch: watch whose handle to remove @@ -734,18 +757,14 @@ virNetDevMacVLanVPortProfileCallback(unsigned char *msg, * * This function is called when a netlink message handler is terminated. * The function frees locally allocated data referenced in the opaque - * data. + * data, and the opaque object itself. */ static void virNetDevMacVLanVPortProfileDestroyCallback(int watch ATTRIBUTE_UNUSED, const unsigned char *macaddr ATTRIBUTE_UNUSED, void *opaque) { - virNetlinkCallbackDataPtr calld = opaque; - - if (calld) { - VIR_FREE(calld->cr_ifname); - } + virNetlinkCallbackDataFree((virNetlinkCallbackDataPtr)opaque); } @@ -789,6 +808,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, int retries, do_retry = 0; uint32_t macvtapMode; const char *cr_ifname; + virNetlinkCallbackDataPtr calld = NULL; int ret; macvtapMode = modeMap[mode]; @@ -894,30 +914,35 @@ create_name: } if (virNetlinkEventServiceIsRunning()) { - virNetlinkCallbackDataPtr calld; + if (VIR_ALLOC(calld) < 0) + goto memory_error; + if ((calld->cr_ifname = strdup(cr_ifname)) == NULL) + goto memory_error; + if (VIR_ALLOC(calld->virtPortProfile) < 0) + goto memory_error; + memcpy(calld->virtPortProfile, virtPortProfile, sizeof(*virtPortProfile)); + if (VIR_ALLOC_N(calld->macaddress, VIR_MAC_BUFLEN) < 0) + goto memory_error; + memcpy(calld->macaddress, macaddress, VIR_MAC_BUFLEN); + if ((calld->linkdev = strdup(linkdev)) == NULL) + goto memory_error; + if (VIR_ALLOC_N(calld->vmuuid, VIR_UUID_BUFLEN) < 0) + goto memory_error; + memcpy(calld->vmuuid, vmuuid, VIR_UUID_BUFLEN); - if (VIR_ALLOC(calld) < 0) { - virReportOOMError(); - goto disassociate_exit; - } - - calld->cr_ifname=strdup(cr_ifname); - if (calld->cr_ifname == NULL) { - virReportOOMError(); - goto disassociate_exit; - } - - calld->virtPortProfile = virtPortProfile; - calld->macaddress = macaddress; - calld->linkdev = linkdev; - calld->vmuuid = vmuuid; calld->vmOp = vmOp; - virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback, calld, macaddress); + virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback, + virNetDevMacVLanVPortProfileDestroyCallback, + calld, macaddress); } return rc; + memory_error: + virReportOOMError(); + virNetlinkCallbackDataFree(calld); + disassociate_exit: ignore_value(virNetDevVPortProfileDisassociate(cr_ifname, virtPortProfile, @@ -965,7 +990,7 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, ret = -1; } - virNetlinkEventRemoveClient(0, macaddr, virNetDevMacVLanVPortProfileDestroyCallback); + virNetlinkEventRemoveClient(0, macaddr); return ret; } diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index de6a135..fd6f751 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -53,7 +53,8 @@ /* State for a single netlink event handle */ struct virNetlinkEventHandle { int watch; - virNetlinkEventHandleCallback cb; + virNetlinkEventHandleCallback handleCB; + virNetlinkEventRemoveCallback removeCB; void *opaque; unsigned char macaddr[VIR_MAC_BUFLEN]; int deleted; @@ -182,20 +183,49 @@ error: } static void -virNetlinkEventServerLock(virNetlinkEventSrvPrivatePtr driver) { +virNetlinkEventServerLock(virNetlinkEventSrvPrivatePtr driver) +{ virMutexLock(&driver->lock); } static void -virNetlinkEventServerUnlock(virNetlinkEventSrvPrivatePtr driver) { +virNetlinkEventServerUnlock(virNetlinkEventSrvPrivatePtr driver) +{ virMutexUnlock(&driver->lock); } +/** + * virNetlinkEventRemoveClientPrimitive: + * + * @i: index of the client to remove from the table + * + * This static function does the low level removal of a client from + * the table once its index is known, including calling the remove + * callback (which usually will free resources required by the + * handler). The event server lock *must* be locked before calling + * this function. + * + * assumes success, returns nothing. + */ +static void +virNetlinkEventRemoveClientPrimitive(size_t i) +{ + virNetlinkEventRemoveCallback removeCB = server->handles[i].removeCB; + + if (removeCB) { + (removeCB)(server->handles[i].watch, + server->handles[i].macaddr, + server->handles[i].opaque); + } + server->handles[i].deleted = VIR_NETLINK_HANDLE_DELETED; +} + static void virNetlinkEventCallback(int watch, int fd ATTRIBUTE_UNUSED, int events ATTRIBUTE_UNUSED, - void *opaque) { + void *opaque) +{ virNetlinkEventSrvPrivatePtr srv = opaque; unsigned char *msg; struct sockaddr_nl peer; @@ -219,23 +249,18 @@ virNetlinkEventCallback(int watch, (int)srv->handlesCount, watch); for (i = 0; i < srv->handlesCount; i++) { - if (srv->handles[i].deleted != VIR_NETLINK_HANDLE_VALID) { + if (srv->handles[i].deleted != VIR_NETLINK_HANDLE_VALID) continue; - } - VIR_DEBUG("dispatching client %d.",i); + VIR_DEBUG("dispatching client %d.", i); - virNetlinkEventHandleCallback cb = srv->handles[i].cb; - void *cpopaque = srv->handles[i].opaque; - (cb)( msg, length, &peer, &handled, cpopaque); + (srv->handles[i].handleCB)(msg, length, &peer, &handled, + srv->handles[i].opaque); } - if (!handled) { + if (!handled) VIR_DEBUG("event not handled."); - } - VIR_FREE(msg); - virNetlinkEventServerUnlock(srv); } @@ -248,29 +273,32 @@ virNetlinkEventCallback(int watch, * returns -1 if the monitor cannot be unregistered, 0 upon success */ int -virNetlinkEventServiceStop(void) { +virNetlinkEventServiceStop(void) +{ virNetlinkEventSrvPrivatePtr srv = server; + int i; VIR_INFO("stopping netlink event service"); - if (!server) { + if (!server) return 0; - } virNetlinkEventServerLock(srv); - nl_close(srv->netlinknh); nl_handle_destroy(srv->netlinknh); - virEventRemoveHandle(srv->eventwatch); - server=0; + /* free any remaining clients on the list */ + for (i = 0; i < srv->handlesCount; i++) { + if (srv->handles[i].deleted == VIR_NETLINK_HANDLE_VALID) + virNetlinkEventRemoveClientPrimitive(i); + } + + server = 0; virNetlinkEventServerUnlock(srv); virMutexDestroy(&srv->lock); - VIR_FREE(srv); - return 0; } @@ -282,7 +310,8 @@ virNetlinkEventServiceStop(void) { * returns 'true' if the service is running, 'false' if stopped. */ bool -virNetlinkEventServiceIsRunning(void) { +virNetlinkEventServiceIsRunning(void) +{ return (server != NULL); } @@ -295,14 +324,14 @@ virNetlinkEventServiceIsRunning(void) { * returns -1 if the monitor cannot be registered, 0 upon success */ int -virNetlinkEventServiceStart(void) { +virNetlinkEventServiceStart(void) +{ virNetlinkEventSrvPrivatePtr srv; int fd; int ret = -1; - if (server) { + if (server) return 0; - } VIR_INFO("starting netlink event service"); @@ -311,7 +340,7 @@ virNetlinkEventServiceStart(void) { goto error; } - if ( virMutexInit(&srv->lock) < 0) + if (virMutexInit(&srv->lock) < 0) goto error; virNetlinkEventServerLock(srv); @@ -340,35 +369,34 @@ virNetlinkEventServiceStart(void) { } if (nl_socket_set_nonblocking(srv->netlinknh)) { - netlinkError(errno, - "%s", _("cannot set netlink socket nonblocking")); + netlinkError(errno, "%s", + _("cannot set netlink socket nonblocking")); goto error_server; } if ((srv->eventwatch = virEventAddHandle(fd, - VIR_EVENT_HANDLE_READABLE, - virNetlinkEventCallback, - srv, NULL)) < 0) { + VIR_EVENT_HANDLE_READABLE, + virNetlinkEventCallback, + srv, NULL)) < 0) { netlinkError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to add netlink event handle watch")); - goto error_server; } srv->netlinkfd = fd; - VIR_DEBUG("netlink event listener on fd: %i running",fd); + VIR_DEBUG("netlink event listener on fd: %i running", fd); ret = 0; - server=srv; + server = srv; error_server: - if (ret == -1){ + if (ret < 0) { nl_close(srv->netlinknh); nl_handle_destroy(srv->netlinknh); } error_locked: virNetlinkEventServerUnlock(srv); - if (ret == -1) { + if (ret < 0) { virMutexDestroy(&srv->lock); VIR_FREE(srv); } @@ -379,7 +407,8 @@ error: /** * virNetlinkEventAddClient: * - * @cb: callback to invoke when an event occurs + * @handleCB: callback to invoke when an event occurs + * @removeCB: callback to invoke when removing a client * @opaque: user data to pass to callback * @macaddr: macaddr to store with the data. Used to identify callers. May be null. * @@ -390,18 +419,19 @@ error: * 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; +virNetlinkEventAddClient(virNetlinkEventHandleCallback handleCB, + virNetlinkEventRemoveCallback removeCB, + void *opaque, const unsigned char *macaddr) +{ + int i, r, ret = -1; virNetlinkEventSrvPrivatePtr srv = server; - if ( cb == NULL ) + if (handleCB == NULL) return -1; virNetlinkEventServerLock(srv); - VIR_DEBUG("adding client: %d.",nextWatch); + VIR_DEBUG("adding client: %d.", nextWatch); r = 0; /* first try to re-use deleted free slots */ @@ -414,31 +444,31 @@ virNetlinkEventAddClient(virNetlinkEventHandleCallback cb, /* Resize the eventLoop array if needed */ if (srv->handlesCount == srv->handlesAlloc) { VIR_DEBUG("Used %zu handle slots, adding at least %d more", - srv->handlesAlloc, NETLINK_EVENT_ALLOC_EXTENT); + srv->handlesAlloc, NETLINK_EVENT_ALLOC_EXTENT); if (VIR_RESIZE_N(srv->handles, srv->handlesAlloc, srv->handlesCount, NETLINK_EVENT_ALLOC_EXTENT) < 0) { - result = -1; goto error; } } r = srv->handlesCount++; addentry: - srv->handles[r].watch = nextWatch; - srv->handles[r].cb = cb; - srv->handles[r].opaque = opaque; - srv->handles[r].deleted = VIR_NETLINK_HANDLE_VALID; + srv->handles[r].watch = nextWatch; + srv->handles[r].handleCB = handleCB; + srv->handles[r].removeCB = removeCB; + srv->handles[r].opaque = opaque; + srv->handles[r].deleted = VIR_NETLINK_HANDLE_VALID; if (macaddr) memcpy(srv->handles[r].macaddr, macaddr, VIR_MAC_BUFLEN); + else + memset(srv->handles[r].macaddr, 0, VIR_MAC_BUFLEN); VIR_DEBUG("added client to loop slot: %d. with macaddr ptr=%p", r, macaddr); - result = nextWatch++; - + ret = nextWatch++; error: virNetlinkEventServerUnlock(srv); - - return result; + return ret; } /** @@ -446,7 +476,6 @@ error: * * @watch: watch whose handle to remove * @macaddr: macaddr whose handle to remove - * &cb: callback for the destruction of local data * * Unregister a callback from a netlink monitor. * The handler function referenced will no longer receive netlink messages. @@ -455,14 +484,13 @@ error: * returns -1 if the file handle was not registered, 0 upon success */ int -virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr, - virNetlinkEventRemoveCallback cb) { +virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr) +{ int i; int ret = -1; virNetlinkEventSrvPrivatePtr srv = server; - VIR_DEBUG("removing client watch=%d, mac=%p.", - watch, macaddr); + VIR_DEBUG("removing client watch=%d, mac=%p.", watch, macaddr); if (watch <= 0 && !macaddr) { VIR_WARN("Ignoring invalid netlink client id: %d", watch); @@ -473,34 +501,22 @@ virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr, for (i = 0; i < srv->handlesCount; i++) { if (srv->handles[i].deleted != VIR_NETLINK_HANDLE_VALID) - continue; + continue; - if (watch && srv->handles[i].watch == watch) { - void *cpopaque = srv->handles[i].opaque; - (cb)( watch, macaddr, cpopaque); + if ((watch && srv->handles[i].watch == watch) || + (!watch && + memcmp(macaddr, srv->handles[i].macaddr, VIR_MAC_BUFLEN) == 0)) { - VIR_FREE(srv->handles[i].opaque); - srv->handles[i].deleted = VIR_NETLINK_HANDLE_DELETED; - VIR_DEBUG("removed client: %d by index.", - srv->handles[i].watch); - ret = 0; - goto error; - } - if (!watch && memcmp(macaddr, srv->handles[i].macaddr, VIR_MAC_BUFLEN) == 0) { - void *cpopaque = srv->handles[i].opaque; - (cb)( watch, macaddr, cpopaque); - - VIR_FREE(srv->handles[i].opaque); - srv->handles[i].deleted = VIR_NETLINK_HANDLE_DELETED; - VIR_DEBUG("removed client: %d by mac.", - srv->handles[i].watch); + virNetlinkEventRemoveClientPrimitive(i); + VIR_DEBUG("removed client: %d by %s.", srv->handles[i].watch, + srv->handles[i].watch ? "index" : "mac"); ret = 0; - goto error; + goto cleanup; } } - VIR_DEBUG("client not found to remove."); + VIR_DEBUG("no client found to remove."); -error: +cleanup: virNetlinkEventServerUnlock(srv); return ret; } @@ -524,7 +540,8 @@ int virNetlinkCommand(struct nl_msg *nl_msg ATTRIBUTE_UNUSED, /** * stopNetlinkEventServer: stop the monitor to receive netlink messages for libvirtd */ -int virNetlinkEventServiceStop(void) { +int virNetlinkEventServiceStop(void) +{ netlinkError(VIR_ERR_INTERNAL_ERROR, "%s", # if defined(__linux__) && !defined(HAVE_LIBNL) @@ -536,7 +553,8 @@ int virNetlinkEventServiceStop(void) { /** * startNetlinkEventServer: start a monitor to receive netlink messages for libvirtd */ -int virNetlinkEventServiceStart(void) { +int virNetlinkEventServiceStart(void) +{ # if defined(__linux__) && !defined(HAVE_LIBNL) netlinkError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -548,7 +566,8 @@ int virNetlinkEventServiceStart(void) { /** * virNetlinkEventServiceIsRunning: returns if the netlink event service is running. */ -int virNetlinkEventServiceIsRunning(void) { +int virNetlinkEventServiceIsRunning(void) +{ # if defined(__linux__) && !defined(HAVE_LIBNL) netlinkError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -560,8 +579,10 @@ int virNetlinkEventServiceIsRunning(void) { /** * virNetlinkEventAddClient: register a callback for handling of netlink messages */ -int virNetlinkEventAddClient(virNetlinkEventHandleCallback cb, void *opaque, - const unsigned char *macaddr) { +int virNetlinkEventAddClient(virNetlinkEventHandleCallback cb, + virNetlinkEventRemoveCallback cb, + void *opaque, const unsigned char *macaddr) +{ netlinkError(VIR_ERR_INTERNAL_ERROR, "%s", # if defined(__linux__) && !defined(HAVE_LIBNL) @@ -575,8 +596,8 @@ int virNetlinkEventAddClient(virNetlinkEventHandleCallback cb, void *opaque, /** * virNetlinkEventRemoveClient: unregister a callback from a netlink monitor */ -int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr, - virNetlinkEventRemoveCallback cb) { +int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr) +{ netlinkError(VIR_ERR_INTERNAL_ERROR, "%s", # if defined(__linux__) && !defined(HAVE_LIBNL) diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index b787a8f..75533a3 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -60,12 +60,13 @@ bool virNetlinkEventServiceIsRunning(void); /** * virNetlinkEventAddClient: register a callback for handling of netlink messages */ -int virNetlinkEventAddClient(virNetlinkEventHandleCallback cb, void *opaque, const unsigned char *macaddr); +int virNetlinkEventAddClient(virNetlinkEventHandleCallback handleCB, + virNetlinkEventRemoveCallback removeCB, + void *opaque, const unsigned char *macaddr); /** * virNetlinkEventRemoveClient: unregister a callback from a netlink monitor */ -int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr, - virNetlinkEventRemoveCallback cb); +int virNetlinkEventRemoveClient(int watch, const unsigned char *macaddr); #endif /* __VIR_NETLINK_H__ */ -- 1.7.7.6

On Feb 29, 2012, at 9:51 AM, Laine Stump wrote: > Since I found a couple other problems, but have made you suffer > through > enough back and forth already, I've made some final suggested changes > myself, and am sending a diff patch as a response to this message > (differences from your latest version to mine), as well as doing a > repost of the original two patches with your latest changes and mine > squashed in. > > Please 1) take a look at the changes in my diff patches , then 2) test > the updated versions of the full patches (I'm unable to test beyond > compiling), and send your ACK if they're okay. Once I have your ACK > back, I'll push (I promise, I won't find any new issues *this* > time :-) > ACK. The code tests out fine. One nit I found in the logs was that the removal was indicated by the wrong method: diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index fd6f751..614f8da 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -509,7 +509,7 @@ virNetlinkEventRemoveClient(int watch, const unsigned char * virNetlinkEventRemoveClientPrimitive(i); VIR_DEBUG("removed client: %d by %s.", srv- >handles[i].watch, - srv->handles[i].watch ? "index" : "mac"); + srv->handles[i].watch ? "mac" : "index"); ret = 0; goto cleanup; } -- Thanks for the effort ! Dirk H

On 02/29/2012 08:43 AM, D. Herrendoerfer wrote:
On Feb 29, 2012, at 9:51 AM, Laine Stump wrote:
Since I found a couple other problems, but have made you suffer through enough back and forth already, I've made some final suggested changes myself, and am sending a diff patch as a response to this message (differences from your latest version to mine), as well as doing a repost of the original two patches with your latest changes and mine squashed in.
Please 1) take a look at the changes in my diff patches , then 2) test the updated versions of the full patches (I'm unable to test beyond compiling), and send your ACK if they're okay. Once I have your ACK back, I'll push (I promise, I won't find any new issues *this* time :-)
ACK.
The code tests out fine. One nit I found in the logs was that the removal was indicated by the wrong method:
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index fd6f751..614f8da 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -509,7 +509,7 @@ virNetlinkEventRemoveClient(int watch, const unsigned char *
virNetlinkEventRemoveClientPrimitive(i); VIR_DEBUG("removed client: %d by %s.", srv->handles[i].watch, - srv->handles[i].watch ? "index" : "mac"); + srv->handles[i].watch ? "mac" : "index");
Aha! Actually it's not that it's switched, it's that I stupidly changed the check to use srv->handles[i].watch (what's stored in the object) instead of using watch (which came in the call args). I'll switch that back and push. Thanks for the fast test turnaround!
ret = 0; goto cleanup; } --
Thanks for the effort !
Dirk H
participants (2)
-
D. Herrendoerfer
-
Laine Stump