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@xxxxxxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list