[libvirt] [PATCH 1/4] Improve netlink to support all protocol.

[Tang Chen]

Hi~

On 08/21/2012 11:04 PM, Doug Goldstein wrote:
> On Tue, Aug 21, 2012 at 5:12 AM, Tang Chen<tangchen at 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 at 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;
>>       }
>>
>>