8:13 a.m.
On Jan 26, 2012, at 5:57 PM, Daniel P. Berrange wrote:
On Fri, Jan 20, 2012 at 03:56:26PM +0100, D. Herrendoerfer wrote:
> From: "D. Herrendoerfer" <d.herrendoerfer(a)herrendoerfer.name>
>
> This code adds an event service for netlink messages addressed
> to libvirt and passes the message to registered callback handlers.
> Itself, it makes use of the polling file event service and follows
> a similar design.
>
> Signed-off-by: D. Herrendoerfer <d.herrendoerfer(a)herrendoerfer.name>
> ---
> daemon/Makefile.am | 3 +-
> daemon/libvirtd.c | 7 +
> src/Makefile.am | 1 +
> src/libvirt_private.syms | 7 +
> src/util/netlink-event.c | 363 ++++++++++++++++++++++++++++++++++++
> ++++++++++
> src/util/netlink-event.h | 63 ++++++++
Our current practice is to use a 'vir' prefix on the files,
so I'd just use 'virnetlink.[ch]' for this code.
virnetlink.[ch]. ok.
> diff --git a/daemon/Makefile.am b/daemon/Makefile.am
> index 73a6e1f..d027ff6 100644
> --- a/daemon/Makefile.am
> +++ b/daemon/Makefile.am
> @@ -114,7 +114,8 @@ libvirtd_LDADD += ../src/probes.o
> endif
>
> libvirtd_LDADD += \
> - ../src/libvirt-qemu.la
> + ../src/libvirt-qemu.la \
> + ../src/libvirt_util.la
Don't do this. This is a sign that you need to add some
APIs in src/libvirt_private.syms instead.
>
> if ! WITH_DRIVER_MODULES
> if WITH_QEMU
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index d7a03d7..b118fd0 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -1577,6 +1579,11 @@ int main(int argc, char **argv) {
> goto cleanup;
> }
>
> + /* Register the netlink event service */
> + if (netlinkEventServiceStart() < 0) {
> + VIR_WARN("Netlink service did not start. Netlink events are
> not available.");
> + }
Looking at the code I think it is reasonable to treat this
failure as a hard fail. On linux this should always succeed.
On non-Linux we should be compiling to a no-op variant.
> /* Run event loop. */
> virNetServerRun(srv);
Probably need to call the Stop method too somewhere....
Yes -
> diff --git a/src/util/netlink-event.c b/src/util/netlink-event.c
> new file mode 100644
> index 0000000..7c6746d
> --- /dev/null
> +++ b/src/util/netlink-event.c
> @@ -0,0 +1,363 @@
> +/*
> + * lldpad-event.c: event loop for monitoring netlink messages
> + *
> + * Copyright (C) 2011,2012 IBM Corporation.
> + * Copyright (C) 2011,2012 Dirk Herrendoerfer
s/2011,2012/2011-2012/
> +#define EVENT_DEBUG(fmt, ...) VIR_DEBUG(fmt, __VA_ARGS__)
Don't do this - just use VIR_DEBUG directly.
Agreed, (all of the above)
> +/* State for a single netlink event handle */
> +struct netlinkEventHandle {
> + int watch;
> + netlinkEventHandleCallback cb;
> + void *opaque;
> + unsigned char macaddr[6];
> + int deleted;
> +};
> +
> +/* State for the main netlink event loop */
> +struct netlinkEventLoop {
> + virMutex lock;
> + int handeled;
> + size_t handlesCount;
> + size_t handlesAlloc;
> + struct netlinkEventHandle *handles;
> +};
> +
> +/* Only have one event loop */
> +static struct netlinkEventLoop eventLoop;
> +
> +/* Unique ID for the next netlink watch to be registered */
> +static int nextWatch = 1;
> +
> +/* Allocate extra slots for virEventPollHandle/virEventPollTimeout
> + records in this multiple */
> +#define NETLINK_EVENT_ALLOC_EXTENT 10
> +
> +static netlinkEventSrvPrivatePtr server = 0;
I'm not really seeing the point in having two global structs,
both with their own lock. I'd say we should just have one
struct with all neccessary state in it.
There are three, One is for the event_poll handler, the second
for the Service loop, and the third holds the clients data.
IMHO it is more complicated to merge them.
> + for (i = 0 ; i < eventLoop.handlesCount ; i++) {
> + if (eventLoop.handles[i].deleted) {
> + continue;
> + }
> +
> + VIR_INFO("dispatching client %d.",i);
> +
> + netlinkEventHandleCallback cb = eventLoop.handles[i].cb;
> + void *cpopaque = eventLoop.handles[i].opaque;
> + (cb)( msg, length, &peer, &handeled, cpopaque);
> + }
> +
> + virMutexUnlock(&eventLoop.lock);
> +
> + if (handeled == 0) {
> + VIR_INFO("nobody cared.");
> + }
> +
> + free(msg);
s/free/VIR_FREE/
> +int
> +netlinkEventServiceStop(void) {
> + netlinkEventSrvPrivatePtr srv = server;
> +
> + VIR_INFO("stopping netlink event service");
> +
> + if (server) {
> + errno = EINVAL;
> + return -1;
> + }
IMHO just return 0 here and skip errno.
> +
> + netlinkEventServerLock(srv);
> +
> + nl_close(srv->netlinknh);
> + nl_handle_destroy(srv->netlinknh);
> +
> + virEventRemoveHandle(srv->eventwatch);
> + server=0;
> +
> + netlinkEventServerUnlock(srv);
> + return 0;
> +}
> +int
> +netlinkEventAddClient(netlinkEventHandleCallback cb,
> + void *opaque,
> + const unsigned char *macaddr) {
> + int i;
> +
> + virMutexLock(&eventLoop.lock);
> +
> + VIR_INFO("adding client: %d.",nextWatch);
> +
> + /* first try to re-use deleted free slots */
> + for (i = 0 ; i < eventLoop.handlesCount ; i++) {
> + if (eventLoop.handles[i].deleted == 2) {
> + eventLoop.handles[i].watch = nextWatch;
> + eventLoop.handles[i].cb = cb;
> + eventLoop.handles[i].opaque = opaque;
> + eventLoop.handles[i].deleted = 0;
> + if (!macaddr)
> + memcpy(eventLoop.handles[i].macaddr, macaddr,6);
s/6/VIR_MAC_BUFLEN/
> + memcpy(eventLoop.handles[i].macaddr, macaddr,6);
And again.
> +
> + VIR_INFO("added client to loop slot: %d.",
> (int)eventLoop.handlesCount);
> +
> + eventLoop.handlesCount++;
> +
> +cleanup:
> + virMutexUnlock(&eventLoop.lock);
> +
> + return nextWatch++;
> +}
> +
> +int
> +netlinkEventRemoveClient(int watch, const unsigned char *macaddr) {
> + if (watch == 0 && memcmp(macaddr,
> eventLoop.handles[i].macaddr, 6)) {
And here, etc
> diff --git a/src/util/netlink-event.h b/src/util/netlink-event.h
> new file mode 100644
> index 0000000..da97395
> --- /dev/null
> +++ b/src/util/netlink-event.h
> @@ -0,0 +1,63 @@
> +/*
> + * lldpad-event.h: event loop for monitoring netlink messages
Wrong filename
All agreed,
> + *
> + * Copyright (C) 2011,2012 IBM Corporation.
> + * Copyright (C) 2011,2012 Dirk Herrendoerfer
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later
> version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free
> Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> 02111-1307 USA
> + *
> + * Author: Dirk Herrendoerfer <herrend[at]de[dot]ibm[dot]com>
> + */
> +
> +#ifndef NETLINK_EVENT_CONF_H
> +# define NETLINK_EVENT_CONF_H
> +
> +#include <netlink/netlink.h>
> +
> +#include "internal.h"
> +#include "threads.h"
> +
> +typedef struct _netlinkEventSrvPrivate netlinkEventSrvPrivate;
> +typedef netlinkEventSrvPrivate *netlinkEventSrvPrivatePtr;
> +struct _netlinkEventSrvPrivate {
> + virMutex lock;
> + int eventwatch;
> + int netlinkfd;
> + struct nl_handle *netlinknh;
> +};
This shouldn't be made public, and can be merged into the other
global state struct.
I agree that it should not be public but I still want to keep them
separate.
DirkH
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/
:|
|: http://libvirt.org -o- http://virt-manager.org
:|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/
:|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc
:|