On 07/18/2012 02:22 PM, tangchen wrote: > NOTE: This patch is just for some comments, so that we can try to > improve netlink support in libvirt. > > This patch introduces a new global array servers[MAX_LINKS], > and all the netlink protocol (at most 32 protocols) > can be supportted. > > And also, it creates a NETLINK_KOBJECT_UEVENT socket to listen > to hotplug events. > > Signed-off-by: Tang Chen <tangchen(a)cn.fujitsu.com&gt; > --- > src/libvirt_private.syms | 1 + > src/util/virnetlink.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++ > src/util/virnetlink.h | 5 +++ > 3 files changed, 109 insertions(+) > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 7373281..0ef21d9 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1378,6 +1378,7 @@ virNetlinkEventServiceIsRunning; > virNetlinkEventServiceLocalPid; > virNetlinkEventServiceStop; > virNetlinkEventServiceStart; > +virNetlinkEventServiceStartProtocol; > virNetlinkShutdown; > virNetlinkStartup; > > diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c > index bb0dae9..489c149 100644 > --- a/src/util/virnetlink.c > +++ b/src/util/virnetlink.c > @@ -98,6 +98,7 @@ static int nextWatch = 1; > # define NETLINK_EVENT_ALLOC_EXTENT 10 > > static virNetlinkEventSrvPrivatePtr server = NULL; > +static virNetlinkEventSrvPrivatePtr servers[MAX_LINKS] = {NULL}; > static virNetlinkHandle *placeholder_nlhandle = NULL; > > /* Function definitions */ > @@ -307,6 +308,8 @@ virNetlinkEventCallback(int watch, > return; > } > > + VIR_INFO("%s", msg); > + Is msg always printable? even if so, should be VIR_DEBUG rather than VIR_INFO. > virNetlinkEventServerLock(srv); > > VIR_DEBUG("dispatching to max %d clients, called from event watch %d", > @@ -398,6 +401,106 @@ int virNetlinkEventServiceLocalPid(void) > > > /** > + * virNetlinkEventServiceStartProtocol: > + * > + * start a monitor to receive netlink messages for libvirtd. > + * This registers a netlink socket with the event interface. > + * > + * @protocol: netlink protocol > + * @groups: broadcast groups to join > + * Returns -1 if the monitor cannot be registered, 0 upon success > + */ > +int > +virNetlinkEventServiceStartProtocol(int protocol, int groups) > +{ > + virNetlinkEventSrvPrivatePtr srv; > + int fd; > + int ret = -1; > + > + if (protocol < 0 || protocol >= MAX_LINKS || > + groups < 0 || groups >= 32) { > + return -EINVAL; You should probably log an error here. > + } > + > + if (servers[protocol]) > + return 0; > + > + VIR_INFO("starting netlink event service with protocol %d", protocol); > + > + if (VIR_ALLOC(srv) < 0) { > + virReportOOMError(); > + return -1; > + } > + > + if (virMutexInit(&srv->lock) < 0) { > + VIR_FREE(srv); > + return -1; > + } > + > + virNetlinkEventServerLock(srv); > + > + /* Allocate a new socket and get fd */ > + srv->netlinknh = virNetlinkAlloc(); > + > + if (!srv->netlinknh) { > + virReportSystemError(errno, > + "%s", _("cannot allocate nlhandle for virNetlinkEvent server")); > + goto error_locked; > + } > + > + nl_join_groups(srv->netlinknh, groups); AFAIK nl_join_groups is deprecated and should be replaced by a setsockopt( ..., NETLINK_ADD_MEMBERSHIP,...) Further, I'd recommend to make the group join optional, i.e. something like if (groups) setsockopt(...); and perhaps add error handling. Not sure, but should the add membership not happen after the connect? > + > + if (nl_connect(srv->netlinknh, protocol) < 0) { > + virReportSystemError(errno, > + "%s", _("cannot connect to netlink socket")); > + goto error_server; > + } > + > + fd = nl_socket_get_fd(srv->netlinknh); > + > + if (fd < 0) { > + virReportSystemError(errno, > + "%s", _("cannot get netlink socket fd")); > + goto error_server; > + } > + > + if (nl_socket_set_nonblocking(srv->netlinknh)) { > + virReportSystemError(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; > + servers[protocol] = srv; > + > +error_server: > + if (ret < 0) { > + nl_close(srv->netlinknh); > + virNetlinkFree(srv->netlinknh); > + } > +error_locked: > + virNetlinkEventServerUnlock(srv); > + if (ret < 0) { > + virMutexDestroy(&srv->lock); > + VIR_FREE(srv); > + } > + return ret; > +} > + > + > +/** > * virNetlinkEventServiceStart: > * > * start a monitor to receive netlink messages for libvirtd. > diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h > index 8ec27c9..256f129 100644 > --- a/src/util/virnetlink.h > +++ b/src/util/virnetlink.h > @@ -59,6 +59,11 @@ int virNetlinkEventServiceStop(void); > int virNetlinkEventServiceStart(void); > > /** > + * startNetlinkEventServerProtocol: start a monitor with specified protocol to receive netlink messages for libvirtd > + */ > +int virNetlinkEventServiceStartProtocol(int protocol, int groups); > + > +/** > * virNetlinkEventServiceIsRunning: returns if the netlink event service is running. > */ > bool virNetlinkEventServiceIsRunning(void); > As far as I can tell, it is necessary to implement equivalents of virNetlinkEventAddClient, virNetlinkEventRemoveClient, virNetlinkEventRemoveClientPrimitive, virNetlinkEventServiceStop and virNetlinkEventServiceIsRunning to include the protocol. Generally, you could go all the way and replace the body of virNetlinkEventServiceStart by a call to virNetlinkEventServiceStartProtocol(NETLINK_ROUTE,0); or change all occurrences of the call to use the extended signature, Maybe Eric or some other maintainer wants to comment as well.