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

[tangchen]

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

-- 
Best Regards,
Tang chen