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

D. Herrendoerfer d.herrendoerfer at herrendoerfer.name
Wed Feb 1 14:13:41 UTC 2012


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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120201/41b56461/attachment-0001.htm>


More information about the libvir-list mailing list