[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