8:11 p.m.
Hi~
I didn't make it clear. :)
I meant to modify virNetlinkEventServiceStart(), but as you said, I also need to modify
virNetlinkEventAddClient(), virNetlinkEventRemoveClient(), and so on. It is a lot
of work then.
So I added a virNetlinkEventServiceStartProtocol() and see if you guys have any
comments. :)
If you think this is OK, then I will try to improve all of them.
Thanks.
On 07/19/2012 01:17 AM, Viktor Mihajlovski wrote:
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>
> ---
> 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.
--
Best Regards,
Tang chen