9:33 p.m.
Hi~
On 08/21/2012 11:04 PM, Doug Goldstein wrote:
On Tue, Aug 21, 2012 at 5:12 AM, Tang
Chen<tangchen(a)cn.fujitsu.com> 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(a)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
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;
> }
>
>