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

Re: [libvirt] [PATCH v7] [REPOST] add 802.1Qbh and 802.1Qbg handling



Scott Feldman wrote:
> On 5/25/10 10:24 AM, "Stefan Berger" <stefanb linux vnet ibm com> wrote:
>
>> Reposting due to malformatted patch.
>
> Thanks for fixing the malformed issue.
>
> My testing is done with this version v7 plus the other patches list below.
> No issues.
>
> ACK.
>
>>         [PATCH v10] vepa: parsing for 802.1Qb{g|h} XML
>>         [RFC][PATCH 1/3] vepa+vsi: Introduce dependency on libnl

Since this one is already pushed,

>>         [PATCH v3] Add host UUID (to libvirt capabilities)

I applied the other two, and then v7.
Once I fixed the unrelated "make check" failure,
I ran "make check" through valgrind and determined
that the "definite" leaks thus exposed are benign.
One bunch applies only to a test and the other is here:

 80 bytes in 1 blocks are definitely lost in loss record 19 of 19
   calloc (vg_replace_malloc.c:418)
   virAlloc (memory.c:100)
   virLastErrorObject (virterror.c:277)
   virResetLastError (virterror.c:418)
   vshDeinit (virsh.c:10285)
   main (virsh.c:10482)

ACK.

For reference, I also computed the v6-v7 incremental, reviewed
it and attach it below:

diff --git a/configure.ac b/configure.ac
index 885b0ae..777dddc 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2024,7 +2024,7 @@ dnl netlink library
 LIBNL_CFLAGS=""
 LIBNL_LIBS=""

-if test "$with_macvtap" = "yes" || "$with_virtualport" = "yes"; then
+if test "$with_macvtap" = "yes" || test "$with_virtualport" = "yes"; then
     PKG_CHECK_MODULES([LIBNL], [libnl-1 >= $LIBNL_REQUIRED], [
     ], [
         AC_MSG_ERROR([libnl >= $LIBNL_REQUIRED is required for macvtap support])
diff --git a/src/util/macvtap.c b/src/util/macvtap.c
index 0224c65..ecaa1e6 100644
--- a/src/util/macvtap.c
+++ b/src/util/macvtap.c
@@ -63,6 +63,13 @@

 # define MICROSEC_PER_SEC       (1000 * 1000)

+# define NLMSGBUF_SIZE  256
+# define RATTBUF_SIZE   64
+
+
+# define STATUS_POLL_TIMEOUT_USEC (10 * MICROSEC_PER_SEC)
+# define STATUS_POLL_INTERVL_USEC (MICROSEC_PER_SEC / 8)
+

 static int associatePortProfileId(const char *macvtap_ifname,
                                   const char *linkdev,
@@ -108,7 +115,7 @@ static void nlClose(int fd)
  */
 static
 int nlComm(struct nlmsghdr *nlmsg,
-           char **respbuf, int *respbuflen)
+           char **respbuf, unsigned int *respbuflen)
 {
     int rc = 0;
     struct sockaddr_nl nladdr = {
@@ -180,8 +187,8 @@ err_exit:
  * @respbuf: pointer to pointer where response buffer will be allocated
  * @respbuflen: pointer to integer holding the size of the response buffer
  *      on return of the function.
- * @to_usecs: timeout in microseconds to wait for a success message
- *            to be returned
+ * @timeout_usecs: timeout in microseconds to wait for a success message
+ *                 to be returned
  *
  * Send the given message to the netlink multicast group and receive
  * responses. Skip responses indicating an error and keep on receiving
@@ -190,8 +197,9 @@ err_exit:
  * buffer will be returned.
  */
 static int
-nlCommWaitSuccess(struct nlmsghdr *nlmsg, int nl_groups,
-                  char **respbuf, int *respbuflen, long to_usecs)
+nlCommWaitSuccess(struct nlmsghdr *nlmsg, uint32_t nl_groups,
+                  char **respbuf, unsigned int *respbuflen,
+                  unsigned long long timeout_usecs)
 {
     int rc = 0;
     struct sockaddr_nl nladdr = {
@@ -200,15 +208,13 @@ nlCommWaitSuccess(struct nlmsghdr *nlmsg, int nl_groups,
             .nl_groups = nl_groups,
     };
     int rcvChunkSize = 1024; // expecting less than that
-    int rcvoffset = 0;
+    size_t rcv_offset = 0;
     ssize_t nbytes;
-    int n;
     struct timeval tv = {
-        .tv_sec  = to_usecs / MICROSEC_PER_SEC,
-        .tv_usec = to_usecs % MICROSEC_PER_SEC,
+        .tv_sec  = timeout_usecs / MICROSEC_PER_SEC,
+        .tv_usec = timeout_usecs % MICROSEC_PER_SEC,
     };
-    fd_set rfds;
-    bool gotvalid = false;
+    bool got_valid = false;
     int fd = nlOpen();
     static uint32_t seq = 0x1234;
     uint32_t myseq = seq++;
@@ -230,12 +236,16 @@ nlCommWaitSuccess(struct nlmsghdr *nlmsg, int nl_groups,
         goto err_exit;
     }

-    while (!gotvalid) {
-        rcvoffset = 0;
+    while (!got_valid) {
+
+        rcv_offset = 0;
+
         while (1) {
+            int n;
+            fd_set rfds;
             socklen_t addrlen = sizeof(nladdr);

-            if (VIR_REALLOC_N(*respbuf, rcvoffset+rcvChunkSize) < 0) {
+            if (VIR_REALLOC_N(*respbuf, rcv_offset + rcvChunkSize) < 0) {
                 virReportOOMError();
                 rc = -1;
                 goto err_exit;
@@ -245,12 +255,18 @@ nlCommWaitSuccess(struct nlmsghdr *nlmsg, int nl_groups,
             FD_SET(fd, &rfds);

             n = select(fd + 1, &rfds, NULL, NULL, &tv);
-            if (n == 0) {
+            if (n <= 0) {
+                if (n < 0)
+                    virReportSystemError(errno, "%s",
+                                         _("error in select call"));
+                if (n == 0)
+                    virReportSystemError(ETIMEDOUT, "%s",
+                            _("no valid netlink response was received"));
                 rc = -1;
                 goto err_exit;
             }

-            nbytes = recvfrom(fd, &((*respbuf)[rcvoffset]), rcvChunkSize, 0,
+            nbytes = recvfrom(fd, &((*respbuf)[rcv_offset]), rcvChunkSize, 0,
                               (struct sockaddr *)&nladdr, &addrlen);
             if (nbytes < 0) {
                 if (errno == EAGAIN || errno == EINTR)
@@ -260,10 +276,10 @@ nlCommWaitSuccess(struct nlmsghdr *nlmsg, int nl_groups,
                 rc = -1;
                 goto err_exit;
             }
-            rcvoffset += nbytes;
+            rcv_offset += nbytes;
             break;
         }
-        *respbuflen = rcvoffset;
+        *respbuflen = rcv_offset;

         /* check message for error */
         if (*respbuflen > NLMSG_LENGTH(0) && *respbuf != NULL) {
@@ -282,25 +298,23 @@ nlCommWaitSuccess(struct nlmsghdr *nlmsg, int nl_groups,
                case NLMSG_ERROR:
                   err = (struct nlmsgerr *)NLMSG_DATA(resp);
                   if (resp->nlmsg_len >= NLMSG_LENGTH(sizeof(*err))) {
-                      if (-err->error != EOPNOTSUPP) {
+                      if (err->error != -EOPNOTSUPP) {
                           /* assuming error msg from daemon */
-                          gotvalid = true;
+                          got_valid = true;
                           break;
                       }
                   }
                   /* whatever this is, skip it */
                   VIR_FREE(*respbuf);
-                  *respbuf = NULL;
                   *respbuflen = 0;
                   break;

                case NLMSG_DONE:
-                  gotvalid = true;
+                  got_valid = true;
                   break;

                default:
                   VIR_FREE(*respbuf);
-                  *respbuf = NULL;
                   *respbuflen = 0;
                   break;
             }
@@ -310,7 +324,6 @@ nlCommWaitSuccess(struct nlmsghdr *nlmsg, int nl_groups,
 err_exit:
     if (rc == -1) {
         VIR_FREE(*respbuf);
-        *respbuf = NULL;
         *respbuflen = 0;
     }

@@ -376,15 +389,15 @@ link_add(const char *type,
          int *retry)
 {
     int rc = 0;
-    char nlmsgbuf[256];
+    char nlmsgbuf[NLMSGBUF_SIZE];
     struct nlmsghdr *nlm = (struct nlmsghdr *)nlmsgbuf, *resp;
     struct nlmsgerr *err;
-    char rtattbuf[64];
+    char rtattbuf[RATTBUF_SIZE];
     struct rtattr *rta, *rta1, *li;
-    struct ifinfomsg i = { .ifi_family = AF_UNSPEC };
+    struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
     int ifindex;
     char *recvbuf = NULL;
-    int recvbuflen;
+    unsigned int recvbuflen;

     if (ifaceGetIndex(true, srcdev, &ifindex) != 0)
         return -1;
@@ -395,65 +408,46 @@ link_add(const char *type,

     nlInit(nlm, NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL, RTM_NEWLINK);

-    if (!nlAppend(nlm, sizeof(nlmsgbuf), &i, sizeof(i)))
+    if (!nlAppend(nlm, sizeof(nlmsgbuf), &ifinfo, sizeof(ifinfo)))
         goto buffer_too_small;

     rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_LINK,
                        &ifindex, sizeof(ifindex));
-    if (!rta)
-        goto buffer_too_small;
-
-    if (!nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
+    if (!rta || !nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
         goto buffer_too_small;

     rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_ADDRESS,
                        macaddress, macaddrsize);
-    if (!rta)
-        goto buffer_too_small;
-
-    if (!nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
+    if (!rta || !nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
         goto buffer_too_small;

     if (ifname) {
         rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_IFNAME,
                            ifname, strlen(ifname) + 1);
-        if (!rta)
-            goto buffer_too_small;
-
-        if (!nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
+        if (!rta || !nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
             goto buffer_too_small;
     }

     rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_LINKINFO, NULL, 0);
-    if (!rta)
-        goto buffer_too_small;
-
-    if (!(li = nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len)))
+    if (!rta ||
+        !(li = nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len)))
         goto buffer_too_small;

     rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_INFO_KIND,
                        type, strlen(type));
-    if (!rta)
-        goto buffer_too_small;
-
-    if (!nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
+    if (!rta || !nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
         goto buffer_too_small;

     if (macvlan_mode > 0) {
         rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_INFO_DATA,
                            NULL, 0);
-        if (!rta)
-            goto buffer_too_small;
-
-        if (!(rta1 = nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len)))
+        if (!rta ||
+            !(rta1 = nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len)))
             goto buffer_too_small;

         rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_MACVLAN_MODE,
                            &macvlan_mode, sizeof(macvlan_mode));
-        if (!rta)
-            goto buffer_too_small;
-
-        if (!nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
+        if (!rta || !nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
             goto buffer_too_small;

         rta1->rta_len = (char *)nlm + nlm->nlmsg_len - (char *)rta1;
@@ -475,15 +469,15 @@ link_add(const char *type,
         if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
             goto malformed_resp;

-        switch (-err->error) {
+        switch (err->error) {

         case 0:
-        break;
+            break;

-        case EEXIST:
+        case -EEXIST:
             *retry = 1;
             rc = -1;
-        break;
+            break;

         default:
             virReportSystemError(-err->error,
@@ -491,10 +485,10 @@ link_add(const char *type,
                                  type);
             rc = -1;
         }
-    break;
+        break;

     case NLMSG_DONE:
-    break;
+        break;

     default:
         goto malformed_resp;
@@ -521,14 +515,14 @@ static int
 link_del(const char *name)
 {
     int rc = 0;
-    char nlmsgbuf[256];
+    char nlmsgbuf[NLMSGBUF_SIZE];
     struct nlmsghdr *nlm = (struct nlmsghdr *)nlmsgbuf, *resp;
     struct nlmsgerr *err;
-    char rtattbuf[64];
+    char rtattbuf[RATTBUF_SIZE];
     struct rtattr *rta;
     struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
     char *recvbuf = NULL;
-    int recvbuflen;
+    unsigned int recvbuflen;

     memset(&nlmsgbuf, 0, sizeof(nlmsgbuf));

@@ -539,10 +533,7 @@ link_del(const char *name)

     rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_IFNAME,
                        name, strlen(name)+1);
-    if (!rta)
-        goto buffer_too_small;
-
-    if (!nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
+    if (!rta || !nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
         goto buffer_too_small;

     if (nlComm(nlm, &recvbuf, &recvbuflen) < 0)
@@ -559,20 +550,16 @@ link_del(const char *name)
         if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
             goto malformed_resp;

-        switch (-err->error) {
-        case 0:
-        break;
-
-        default:
+        if (err->error) {
             virReportSystemError(-err->error,
                                  _("error destroying %s interface"),
                                  name);
             rc = -1;
         }
-    break;
+        break;

     case NLMSG_DONE:
-    break;
+        break;

     default:
         goto malformed_resp;
@@ -672,11 +659,9 @@ macvtapModeFromInt(enum virDomainNetdevMacvtapType mode)
     switch (mode) {
     case VIR_DOMAIN_NETDEV_MACVTAP_MODE_PRIVATE:
         return MACVLAN_MODE_PRIVATE;
-    break;

     case VIR_DOMAIN_NETDEV_MACVTAP_MODE_BRIDGE:
         return MACVLAN_MODE_BRIDGE;
-    break;

     case VIR_DOMAIN_NETDEV_MACVTAP_MODE_VEPA:
     default:
@@ -910,20 +895,20 @@ static int
 link_dump(bool multicast, int ifindex, struct nlattr **tb, char **recvbuf)
 {
     int rc = 0;
-    char nlmsgbuf[256] = { 0, };
+    char nlmsgbuf[NLMSGBUF_SIZE] = { 0, };
     struct nlmsghdr *nlm = (struct nlmsghdr *)nlmsgbuf, *resp;
     struct nlmsgerr *err;
-    struct ifinfomsg i = {
+    struct ifinfomsg ifinfo = {
         .ifi_family = AF_UNSPEC,
         .ifi_index  = ifindex
     };
-    int recvbuflen;
+    unsigned int recvbuflen;

     *recvbuf = NULL;

     nlInit(nlm, NLM_F_REQUEST, RTM_GETLINK);

-    if (!nlAppend(nlm, sizeof(nlmsgbuf), &i, sizeof(i)))
+    if (!nlAppend(nlm, sizeof(nlmsgbuf), &ifinfo, sizeof(ifinfo)))
         goto buffer_too_small;

     if (!multicast) {
@@ -946,17 +931,13 @@ link_dump(bool multicast, int ifindex, struct nlattr **tb, char **recvbuf)
         if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
             goto malformed_resp;

-        switch (-err->error) {
-        case 0:
-        break;
-
-        default:
+        if (err->error) {
             virReportSystemError(-err->error,
                                  _("error dumping %d interface"),
                                  ifindex);
             rc = -1;
         }
-    break;
+        break;

     case GENL_ID_CTRL:
     case NLMSG_DONE:
@@ -964,7 +945,7 @@ link_dump(bool multicast, int ifindex, struct nlattr **tb, char **recvbuf)
                         tb, IFLA_MAX, ifla_policy)) {
             goto malformed_resp;
         }
-    break;
+        break;

     default:
         goto malformed_resp;
@@ -1048,17 +1029,17 @@ doPortProfileOpSetLink(bool multicast,
                        uint8_t op)
 {
     int rc = 0;
-    char nlmsgbuf[256];
+    char nlmsgbuf[NLMSGBUF_SIZE];
     struct nlmsghdr *nlm = (struct nlmsghdr *)nlmsgbuf, *resp;
     struct nlmsgerr *err;
-    char rtattbuf[64];
+    char rtattbuf[RATTBUF_SIZE];
     struct rtattr *rta, *vfports = NULL, *vfport;
     struct ifinfomsg ifinfo = {
         .ifi_family = AF_UNSPEC,
         .ifi_index  = ifindex,
     };
     char *recvbuf = NULL;
-    int recvbuflen = 0;
+    unsigned int recvbuflen = 0;

     memset(&nlmsgbuf, 0, sizeof(nlmsgbuf));

@@ -1071,79 +1052,57 @@ doPortProfileOpSetLink(bool multicast,
         rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_PORT_SELF, NULL, 0);
     } else {
         rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_VF_PORTS, NULL, 0);
-        if (!rta)
-            goto buffer_too_small;
-
-        if (!(vfports = nlAppend(nlm, sizeof(nlmsgbuf),
+        if (!rta ||
+            !(vfports = nlAppend(nlm, sizeof(nlmsgbuf),
                                  rtattbuf, rta->rta_len)))
             goto buffer_too_small;

-        /* beging nesting vfports */
+        /* begin nesting vfports */
         rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_VF_PORT, NULL, 0);
     }

-    if (!rta)
-        goto buffer_too_small;
-
-    if (!(vfport = nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len)))
+    if (!rta ||
+        !(vfport = nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len)))
         goto buffer_too_small;

     if (profileId) {
         rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_PORT_PROFILE,
                            profileId, strlen(profileId) + 1);
-        if (!rta)
-            goto buffer_too_small;
-
-        if (!nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
+        if (!rta || !nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
             goto buffer_too_small;
     }

     if (portVsi) {
         rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_PORT_VSI_TYPE,
                            portVsi, sizeof(*portVsi));
-        if (!rta)
-            goto buffer_too_small;
-
-        if (!nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
+        if (!rta || !nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
             goto buffer_too_small;
     }

     if (instanceId) {
         rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_PORT_INSTANCE_UUID,
                            instanceId, VIR_UUID_BUFLEN);
-        if (!rta)
-            goto buffer_too_small;
-
-        if (!nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
+        if (!rta || !nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
             goto buffer_too_small;
     }

     if (hostUUID) {
         rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_PORT_HOST_UUID,
                            hostUUID, VIR_UUID_BUFLEN);
-        if (!rta)
-            goto buffer_too_small;
-
-        if (!nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
+        if (!rta || !nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
             goto buffer_too_small;
     }

     if (vf != PORT_SELF_VF) {
         rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_PORT_VF,
                            &vf, sizeof(vf));
-        if (!rta)
-            goto buffer_too_small;
-
-        if (!nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
+        if (!rta || !nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
             goto buffer_too_small;
     }

     rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_PORT_REQUEST,
                        &op, sizeof(op));
-    if (!rta)
-        goto buffer_too_small;
-
-    if (!nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
+    if (!rta || !nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
         goto buffer_too_small;

     /* end nesting of vport */
@@ -1174,20 +1133,16 @@ doPortProfileOpSetLink(bool multicast,
         if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
             goto malformed_resp;

-        switch (-err->error) {
-        case 0:
-        break;
-
-        default:
+        if (err->error) {
             virReportSystemError(-err->error,
                 _("error during virtual port configuration of ifindex %d"),
                 ifindex);
             rc = -1;
         }
-    break;
+        break;

     case NLMSG_DONE:
-    break;
+        break;

     default:
         goto malformed_resp;
@@ -1223,7 +1178,7 @@ doPortProfileOpCommon(bool multicast,
     int rc;
     char *recvbuf = NULL;
     struct nlattr *tb[IFLA_MAX + 1];
-    int repeats = 80;
+    int repeats = STATUS_POLL_TIMEOUT_USEC / STATUS_POLL_INTERVL_USEC;
     uint16_t status = 0;

     rc = doPortProfileOpSetLink(multicast,
@@ -1241,7 +1196,7 @@ doPortProfileOpCommon(bool multicast,
         return rc;
     }

-    while (--repeats) {
+    while (--repeats >= 0) {
         rc = link_dump(multicast, ifindex, tb, &recvbuf);
         if (rc)
             goto err_exit;
@@ -1259,8 +1214,10 @@ doPortProfileOpCommon(bool multicast,
                 rc = 1;
                 break;
             }
-        }
-        usleep(125000);
+        } else
+            goto err_exit;
+
+        usleep(STATUS_POLL_INTERVL_USEC);

         VIR_FREE(recvbuf);
     }


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