[libvirt] [RFC] [PATCH] Support for uevent netlink and hotplug event.

[Viktor Mihajlovski]

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

-- 

Mit freundlichen Grüßen/Kind Regards
   Viktor Mihajlovski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294