[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/2] Add netlink message event service



On Fri, Jan 20, 2012 at 03:56:26PM +0100, D. Herrendoerfer wrote:
> From: "D. Herrendoerfer" <d herrendoerfer herrendoerfer name>
> 
> This code adds an event service for netlink messages addressed
> to libvirt and passes the message to registered callback handlers.
> Itself, it makes use of the polling file event service and follows
> a similar design.
> 
> Signed-off-by: D. Herrendoerfer <d herrendoerfer herrendoerfer name>
> ---
>  daemon/Makefile.am       |    3 +-
>  daemon/libvirtd.c        |    7 +
>  src/Makefile.am          |    1 +
>  src/libvirt_private.syms |    7 +
>  src/util/netlink-event.c |  363 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/netlink-event.h |   63 ++++++++

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

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

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

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

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

> + *
> + * 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.

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]