[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



Hi~

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




[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]