Hi~ On 08/21/2012 11:04 PM, Doug Goldstein wrote:
This patch improve all the API in virnetlink.c to support all kinds of netlink protocols, and make all netlink sockets be able to join in groups.
SOL_NETLINK is not defined on every system, so this patch defines SOL_NETLINK as 270.
Signed-off-by: Tang Chen<tangchen@cn.fujitsu.com> --- The commit message is a bit weak on what exactly is being added. The most details are about a 3 line change. OK, I will fix this. :) -static virNetlinkEventSrvPrivatePtr server = NULL; +static virNetlinkEventSrvPrivatePtr server[MAX_LINKS] = {NULL}; static virNetlinkHandle *placeholder_nlhandle = NULL; Maybe a little code comment as to why MAX_LINKS. e.g. The Linux kernel supports up to MAX_LINKS (32 at the time) individual netlink
On Tue, Aug 21, 2012 at 5:12 AM, Tang Chen<tangchen@cn.fujitsu.com> wrote: protocols. Sure, I will add some comments. :)
/* Function definitions */ @@ -158,6 +163,7 @@ virNetlinkShutdown(void) * @respbuflen: pointer to integer holding the size of the response buffer * on return of the function. * @nl_pid: the pid of the process to talk to, i.e., pid = 0 for kernel + * @protocol: netlink protocol * * Send the given message to the netlink layer and receive response. * Returns 0 on success, -1 on error. In case of error, no response Function documentation doesn't match your implementation below. I will fix it. :)
@@ -165,7 +171,8 @@ virNetlinkShutdown(void) */ int virNetlinkCommand(struct nl_msg *nl_msg, unsigned char **respbuf, unsigned int *respbuflen, - uint32_t src_pid, uint32_t dst_pid) + uint32_t src_pid, uint32_t dst_pid, + unsigned int protocol, unsigned int groups) { int rc = 0; struct sockaddr_nl nladdr = { @@ -181,17 +188,46 @@ int virNetlinkCommand(struct nl_msg *nl_msg, int fd; int n; struct nlmsghdr *nlmsg = nlmsg_hdr(nl_msg); - virNetlinkHandle *nlhandle = virNetlinkAlloc(); + virNetlinkHandle *nlhandle = NULL; + + if (protocol>= MAX_LINKS) { + virReportSystemError(EINVAL, + _("invalid protocol argument: %d"), protocol); + return -EINVAL; + } + + if (groups>= 32) { I believe there is a define for this in the headers so it would be better to use that then hardcoding a number without any code comments to what it means. If there's not a define, I would at least document what the 32 is and where it derives from the kernel code so that if things change in the future it will be easier to fix. I found that the document of nl_socket_add_membership() said the following:
"Joins the specified group using the modern socket option which is available since kernel version 2.6.14. It allows joining an almost arbitary number of groups without limitation." So maybe a check for "groups" is not necessary. Can we just replace setsockopt() with nl_socket_add_membership() and leave the checking and error handling in nl_socket_add_membership() ?
+ virReportSystemError(EINVAL, + _("invalid group argument: %d"), groups); + return -EINVAL; + }
+ nlhandle = virNetlinkAlloc(); if (!nlhandle) { virReportSystemError(errno, "%s", _("cannot allocate nlhandle for netlink")); return -1; }
- if (nl_connect(nlhandle, NETLINK_ROUTE)< 0) { + if (nl_connect(nlhandle, protocol)< 0) { virReportSystemError(errno, - "%s", _("cannot connect to netlink socket")); + _("cannot connect to netlink socket with protocol %d"), protocol); + rc = -1; + goto error; + } + + fd = nl_socket_get_fd(nlhandle); + if (fd< 0) { + virReportSystemError(errno, + "%s", _("cannot get netlink socket fd")); + rc = -1; + goto error; + } + + if (groups&& setsockopt(fd, SOL_NETLINK, NETLINK_ADD_MEMBERSHIP, +&groups, sizeof(groups))< 0) { + virReportSystemError(errno, + "%s", _("cannot add netlink membership")); rc = -1; goto error; } @@ -208,8 +244,6 @@ int virNetlinkCommand(struct nl_msg *nl_msg, goto error; }