[libvirt PATCH v6 1/3] Set VF MAC and VLAN ID in two different operations

Dmitrii Shcherbakov dmitrii.shcherbakov at canonical.com
Thu Nov 18 17:15:18 UTC 2021


This has a benefit of being able to handle error codes for those
operations separately which is useful when drivers allow setting a MAC
address but do not allow setting a VLAN (which is the case with some
SmartNIC DPUs).

Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov at canonical.com>
---
 src/libvirt_private.syms |   7 ++
 src/util/virnetdev.c     | 188 ++++++++++++++++++++-------------
 src/util/virnetdevpriv.h |  44 ++++++++
 tests/virnetdevtest.c    | 222 ++++++++++++++++++++++++++++++++++++++-
 4 files changed, 388 insertions(+), 73 deletions(-)
 create mode 100644 src/util/virnetdevpriv.h

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a7bc50a4d1..e681e69d77 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2849,6 +2849,13 @@ virNetDevOpenvswitchSetTimeout;
 virNetDevOpenvswitchUpdateVlan;
 
 
+#util/virnetdevpriv.h
+virNetDevSendVfSetLinkRequest;
+virNetDevSetVfConfig;
+virNetDevSetVfMac;
+virNetDevSetVfVlan;
+
+
 # util/virnetdevtap.h
 virNetDevTapAttachBridge;
 virNetDevTapCreate;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 58f7360a0f..f2f5fa2d95 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -19,7 +19,9 @@
 #include <config.h>
 #include <math.h>
 
-#include "virnetdev.h"
+#define LIBVIRT_VIRNETDEVPRIV_H_ALLOW
+
+#include "virnetdevpriv.h"
 #include "viralloc.h"
 #include "virnetlink.h"
 #include "virmacaddr.h"
@@ -1527,16 +1529,12 @@ static struct nla_policy ifla_vfstats_policy[IFLA_VF_STATS_MAX+1] = {
     [IFLA_VF_STATS_MULTICAST]   = { .type = NLA_U64 },
 };
 
-
-static int
-virNetDevSetVfConfig(const char *ifname, int vf,
-                     const virMacAddr *macaddr, int vlanid,
-                     bool *allowRetry)
+int
+virNetDevSendVfSetLinkRequest(const char *ifname, int vfInfoType,
+                              const void *payload, const size_t payloadLen)
 {
-    int rc = -1;
-    char macstr[VIR_MAC_STRING_BUFLEN];
     g_autofree struct nlmsghdr *resp = NULL;
-    struct nlmsgerr *err;
+    struct nlmsgerr *err = NULL;
     unsigned int recvbuflen = 0;
     struct nl_msg *nl_msg;
     struct nlattr *vfinfolist, *vfinfo;
@@ -1544,9 +1542,11 @@ virNetDevSetVfConfig(const char *ifname, int vf,
         .ifi_family = AF_UNSPEC,
         .ifi_index  = -1,
     };
+    int rc = 0;
 
-    if (!macaddr && vlanid < 0)
+    if (payload == NULL || payloadLen == 0) {
         return -1;
+    }
 
     nl_msg = virNetlinkMsgNew(RTM_SETLINK, NLM_F_REQUEST);
 
@@ -1564,30 +1564,8 @@ virNetDevSetVfConfig(const char *ifname, int vf,
     if (!(vfinfo = nla_nest_start(nl_msg, IFLA_VF_INFO)))
         goto buffer_too_small;
 
-    if (macaddr) {
-        struct ifla_vf_mac ifla_vf_mac = {
-             .vf = vf,
-             .mac = { 0, },
-        };
-
-        virMacAddrGetRaw(macaddr, ifla_vf_mac.mac);
-
-        if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac),
-                    &ifla_vf_mac) < 0)
-            goto buffer_too_small;
-    }
-
-    if (vlanid >= 0) {
-        struct ifla_vf_vlan ifla_vf_vlan = {
-             .vf = vf,
-             .vlan = vlanid,
-             .qos = 0,
-        };
-
-        if (nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan),
-                    &ifla_vf_vlan) < 0)
-            goto buffer_too_small;
-    }
+    if (nla_put(nl_msg, vfInfoType, payloadLen, payload) < 0)
+        goto buffer_too_small;
 
     nla_nest_end(nl_msg, vfinfo);
     nla_nest_end(nl_msg, vfinfolist);
@@ -1600,48 +1578,20 @@ virNetDevSetVfConfig(const char *ifname, int vf,
         goto malformed_resp;
 
     switch (resp->nlmsg_type) {
-    case NLMSG_ERROR:
-        err = (struct nlmsgerr *)NLMSG_DATA(resp);
-        if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
+        case NLMSG_ERROR:
+            err = (struct nlmsgerr *)NLMSG_DATA(resp);
+            if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
+                goto malformed_resp;
+            rc = err->error;
+            break;
+        case NLMSG_DONE:
+            rc = 0;
+            break;
+        default:
             goto malformed_resp;
-
-        /* if allowRetry is true and the error was EINVAL, then
-         * silently return a failure so the caller can retry with a
-         * different MAC address
-         */
-        if (err->error == -EINVAL && *allowRetry &&
-            macaddr && !virMacAddrCmp(macaddr, &zeroMAC)) {
-            goto cleanup;
-        } else if (err->error) {
-            /* other errors are permanent */
-            virReportSystemError(-err->error,
-                                 _("Cannot set interface MAC/vlanid to %s/%d "
-                                   "for ifname %s vf %d"),
-                                 (macaddr
-                                  ? virMacAddrFormat(macaddr, macstr)
-                                  : "(unchanged)"),
-                                 vlanid,
-                                 ifname ? ifname : "(unspecified)",
-                                 vf);
-            *allowRetry = false; /* no use retrying */
-            goto cleanup;
-        }
-        break;
-
-    case NLMSG_DONE:
-        break;
-
-    default:
-        goto malformed_resp;
     }
 
-    rc = 0;
  cleanup:
-    VIR_DEBUG("RTM_SETLINK %s vf %d MAC=%s vlanid=%d - %s",
-              ifname, vf,
-              macaddr ? virMacAddrFormat(macaddr, macstr) : "(unchanged)",
-              vlanid, rc < 0 ? "Fail" : "Success");
-
     nlmsg_free(nl_msg);
     return rc;
 
@@ -1656,6 +1606,100 @@ virNetDevSetVfConfig(const char *ifname, int vf,
     goto cleanup;
 }
 
+int
+virNetDevSetVfVlan(const char *ifname, int vf, int vlanid)
+{
+    int rc = 0;
+    int requestError = 0;
+
+    struct ifla_vf_vlan ifla_vf_vlan = {
+         .vf = vf,
+         .vlan = vlanid,
+         .qos = 0,
+    };
+
+    /* VLAN ids 0 and 4095 are reserved per 802.1Q but are valid values. */
+    if ((vlanid < 0 || vlanid > 4095)) {
+        return -ERANGE;
+    }
+
+    requestError = virNetDevSendVfSetLinkRequest(ifname, IFLA_VF_VLAN,
+                                                 &ifla_vf_vlan, sizeof(ifla_vf_vlan));
+
+    if (requestError) {
+        virReportSystemError(-requestError,
+                             _("Cannot set interface vlanid to %d "
+                               "for ifname %s vf %d"),
+                             vlanid, ifname ? ifname : "(unspecified)", vf);
+        rc = requestError;
+    }
+    VIR_DEBUG("RTM_SETLINK %s vf %d vlanid=%d - %s",
+              ifname, vf,
+              vlanid, rc < 0 ? "Fail" : "Success");
+    return rc;
+}
+
+int
+virNetDevSetVfMac(const char *ifname, int vf,
+                  const virMacAddr *macaddr,
+                  bool *allowRetry)
+{
+    int rc = 0;
+    int requestError = 0;
+    char macstr[VIR_MAC_STRING_BUFLEN];
+
+    struct ifla_vf_mac ifla_vf_mac = {
+         .vf = vf,
+         .mac = { 0, },
+    };
+
+    if (macaddr == NULL || allowRetry == NULL)
+        return -EINVAL;
+
+    virMacAddrGetRaw(macaddr, ifla_vf_mac.mac);
+
+    requestError = virNetDevSendVfSetLinkRequest(ifname, IFLA_VF_MAC,
+                                                 &ifla_vf_mac, sizeof(ifla_vf_mac));
+    /* if allowRetry is true and the error was EINVAL, then
+     * silently return a failure so the caller can retry with a
+     * different MAC address. */
+    if (requestError == -EINVAL && *allowRetry && !virMacAddrCmp(macaddr, &zeroMAC)) {
+        rc = requestError;
+    } else if (requestError) {
+        /* other errors are permanent */
+        virReportSystemError(-requestError,
+                             _("Cannot set interface MAC to %s "
+                               "for ifname %s vf %d"),
+                             (macaddr
+                              ? virMacAddrFormat(macaddr, macstr)
+                              : "(unchanged)"),
+                             ifname ? ifname : "(unspecified)",
+                             vf);
+        *allowRetry = false; /* no use retrying */
+        rc = requestError;
+    } else {
+        rc = 0;
+    }
+    VIR_DEBUG("RTM_SETLINK %s vf %d MAC=%s - %s",
+              ifname, vf,
+              macaddr ? virMacAddrFormat(macaddr, macstr) : "(unchanged)",
+              rc < 0 ? "Fail" : "Success");
+    return rc;
+}
+
+int
+virNetDevSetVfConfig(const char *ifname, int vf,
+                     const virMacAddr *macaddr, int vlanid,
+                     bool *allowRetry)
+{
+    int rc = 0;
+    if ((rc = virNetDevSetVfMac(ifname, vf, macaddr, allowRetry)) < 0)
+        return rc;
+    if ((rc = virNetDevSetVfVlan(ifname, vf, vlanid)) < 0)
+        return rc;
+    return rc;
+}
+
 /**
  * virNetDevParseVfInfo:
  * Get the VF interface information from kernel by netlink, To make netlink
diff --git a/src/util/virnetdevpriv.h b/src/util/virnetdevpriv.h
new file mode 100644
index 0000000000..7990bf5269
--- /dev/null
+++ b/src/util/virnetdevpriv.h
@@ -0,0 +1,44 @@
+/*
+ * virnetdevpriv.h: private virnetdev header for unit testing
+ *
+ * Copyright (C) 2021 Canonical Ltd.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library;  If not, see
+ * <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef LIBVIRT_VIRNETDEVPRIV_H_ALLOW
+# error "virnetdevpriv.h may only be included by virnetdev.c or test suites"
+#endif /* LIBVIRT_VIRNETDEVPRIV_H_ALLOW */
+
+#pragma once
+
+#include "virnetdev.h"
+
+int
+virNetDevSendVfSetLinkRequest(const char *ifname, int vfInfoType,
+                              const void *payload, const size_t payloadLen);
+
+int
+virNetDevSetVfVlan(const char *ifname, int vf, int vlanid);
+
+int
+virNetDevSetVfMac(const char *ifname, int vf,
+                  const virMacAddr *macaddr,
+                  bool *allowRetry);
+
+int
+virNetDevSetVfConfig(const char *ifname, int vf,
+                     const virMacAddr *macaddr, int vlanid,
+                     bool *allowRetry);
diff --git a/tests/virnetdevtest.c b/tests/virnetdevtest.c
index aadbeb1ef4..15e8f75d8b 100644
--- a/tests/virnetdevtest.c
+++ b/tests/virnetdevtest.c
@@ -18,11 +18,17 @@
 
 #include <config.h>
 
+#include "internal.h"
 #include "testutils.h"
 
+#include "virnetlink.h"
+
+#define LIBVIRT_VIRNETDEVPRIV_H_ALLOW
+
 #ifdef __linux__
 
-# include "virnetdev.h"
+# include "virmock.h"
+# include "virnetdevpriv.h"
 
 # define VIR_FROM_THIS VIR_FROM_NONE
 
@@ -59,6 +65,211 @@ testVirNetDevGetLinkInfo(const void *opaque)
     return 0;
 }
 
+int
+(*real_virNetDevSendVfSetLinkRequest)(const char *ifname, int vfInfoType,
+                                      const void *payload, const size_t payloadLen);
+
+int
+(*real_virNetDevSetVfMac)(const char *ifname, int vf, const virMacAddr *macaddr, bool *allowRetry);
+
+int
+(*real_virNetDevSetVfVlan)(const char *ifname, int vf, int vlanid);
+
+static void
+init_syms(void)
+{
+    VIR_MOCK_REAL_INIT(virNetDevSendVfSetLinkRequest);
+    VIR_MOCK_REAL_INIT(virNetDevSetVfMac);
+    VIR_MOCK_REAL_INIT(virNetDevSetVfVlan);
+}
+
+int
+virNetDevSetVfMac(const char *ifname, int vf,
+                  const virMacAddr *macaddr,
+                  bool *allowRetry)
+{
+    init_syms();
+
+    if (STREQ_NULLABLE(ifname, "fakeiface-macerror")) {
+        return -EBUSY;
+    } else if (STREQ_NULLABLE(ifname, "fakeiface-altmacerror")) {
+        return -EINVAL;
+    } else if (STREQ_NULLABLE(ifname, "fakeiface-macerror-novlanerror")) {
+        return -EAGAIN;
+    } else if (STREQ_NULLABLE(ifname, "fakeiface-macerror-vlanerror")) {
+        return -ENODEV;
+    } else if (STREQ_NULLABLE(ifname, "fakeiface-nomacerror-vlanerror")) {
+        return 0;
+    } else if (STREQ_NULLABLE(ifname, "fakeiface-nomacerror-novlanerror")) {
+        return 0;
+    }
+    return real_virNetDevSetVfMac(ifname, vf, macaddr, allowRetry);
+}
+
+int
+virNetDevSetVfVlan(const char *ifname, int vf, int vlanid)
+{
+    init_syms();
+
+    if (STREQ_NULLABLE(ifname, "fakeiface-macerror-vlanerror")) {
+        return -EPERM;
+    } else if (STREQ_NULLABLE(ifname, "fakeiface-nomacerror-vlanerror")) {
+        return -EPERM;
+    } else if (STREQ_NULLABLE(ifname, "fakeiface-macerror-novlanerror")) {
+        return 0;
+    } else if (STREQ_NULLABLE(ifname, "fakeiface-nomacerror-novlanerror")) {
+        return 0;
+    }
+    return real_virNetDevSetVfVlan(ifname, vf, vlanid);
+}
+
+int
+virNetDevSendVfSetLinkRequest(const char *ifname, int vfInfoType,
+                              const void *payload, const size_t payloadLen)
+{
+    init_syms();
+
+    if (STREQ_NULLABLE(ifname, "fakeiface-eperm")) {
+        return -EPERM;
+    } else if (STREQ_NULLABLE(ifname, "fakeiface-eagain")) {
+        return -EAGAIN;
+    } else if (STREQ_NULLABLE(ifname, "fakeiface-einval")) {
+        return -EINVAL;
+    } else if (STREQ_NULLABLE(ifname, "fakeiface-ok")) {
+        return 0;
+    }
+    return real_virNetDevSendVfSetLinkRequest(ifname, vfInfoType, payload, payloadLen);
+}
+
+static int
+testVirNetDevSetVfMac(const void *opaque G_GNUC_UNUSED)
+{
+    struct testCase {
+        const char *ifname;
+        const int vf_num;
+        const virMacAddr macaddr;
+        bool allow_retry;
+        const int rc;
+    };
+    size_t i = 0;
+    int rc = 0;
+    struct testCase testCases[] = {
+        { .ifname = "fakeiface-ok", .vf_num = 1,
+          .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = false, .rc = 0 },
+        { .ifname = "fakeiface-ok", .vf_num = 2,
+          .macaddr = { .addr = { 0, 0, 0, 7, 7, 7 } }, .allow_retry = false, .rc = 0 },
+        { .ifname = "fakeiface-ok", .vf_num = 3,
+          .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = true, .rc = 0 },
+        { .ifname = "fakeiface-ok", .vf_num = 4,
+          .macaddr = { .addr = { 0, 0, 0, 7, 7, 7 } }, .allow_retry = true, .rc = 0 },
+        { .ifname = "fakeiface-eperm", .vf_num = 5,
+          .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = false, .rc = -EPERM },
+        { .ifname = "fakeiface-einval", .vf_num = 6,
+          .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = false, .rc = -EINVAL },
+        { .ifname = "fakeiface-einval", .vf_num = 7,
+          .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = true, .rc = -EINVAL },
+        { .ifname = "fakeiface-einval", .vf_num = 8,
+          .macaddr = { .addr = { 0, 0, 0, 7, 7, 7 } }, .allow_retry = false, .rc = -EINVAL },
+        { .ifname = "fakeiface-einval", .vf_num = 9,
+          .macaddr = { .addr = { 0, 0, 0, 7, 7, 7 } }, .allow_retry = true, .rc = -EINVAL },
+    };
+
+    for (i = 0; i < sizeof(testCases) / sizeof(struct testCase); ++i) {
+       rc = virNetDevSetVfMac(testCases[i].ifname, testCases[i].vf_num,
+                              &testCases[i].macaddr, &testCases[i].allow_retry);
+       if (rc != testCases[i].rc) {
+           return -1;
+       }
+    }
+    return 0;
+}
+
+static int
+testVirNetDevSetVfMissingMac(const void *opaque G_GNUC_UNUSED)
+{
+    bool allowRetry = false;
+    /* NULL MAC pointer. */
+    if (virNetDevSetVfMac("fakeiface-ok", 1, NULL, &allowRetry) != -EINVAL) {
+        return -1;
+    }
+    allowRetry = true;
+    if (virNetDevSetVfMac("fakeiface-ok", 1, NULL, &allowRetry) != -EINVAL) {
+        return -1;
+    }
+    return 0;
+}
+
+static int
+testVirNetDevSetVfVlan(const void *opaque G_GNUC_UNUSED)
+{
+    struct testCase {
+        const char *ifname;
+        const int vf_num;
+        const int vlan_id;
+        const int rc;
+    };
+    size_t i = 0;
+    int rc = 0;
+    const struct testCase testCases[] = {
+        /* VLAN ID is out of range of valid values (0-4095). */
+        { .ifname = "enxdeadbeefcafe", .vf_num = 1, .vlan_id = 4096, .rc = -ERANGE },
+        { .ifname = "enxdeadbeefcafe", .vf_num = 1, .vlan_id = -1, .rc = -ERANGE },
+        { .ifname = "fakeiface-eperm", .vf_num = 1, .vlan_id = 0, .rc = -EPERM },
+        { .ifname = "fakeiface-eagain", .vf_num = 1, .vlan_id = 0, .rc = -EAGAIN },
+        /* Successful requests with vlan id 0 need to have a zero return code. */
+        { .ifname = "fakeiface-ok", .vf_num = 1, .vlan_id = 0, .rc = 0 },
+        /* Requests with a non-zero VLAN ID that result in an EPERM need to result in failures.
+         * failures. */
+        { .ifname = "fakeiface-eperm", .vf_num = 1, .vlan_id = 42, .rc = -EPERM },
+        /* Requests with a non-zero VLAN ID that result in some other errors need to result in
+         * failures. */
+        { .ifname = "fakeiface-eagain", .vf_num = 1, .vlan_id = 42, .rc = -EAGAIN },
+        /* Successful requests with a non-zero VLAN ID */
+        { .ifname = "fakeiface-ok", .vf_num = 1, .vlan_id = 42, .rc = 0 },
+    };
+
+    for (i = 0; i < sizeof(testCases) / sizeof(struct testCase); ++i) {
+       rc = virNetDevSetVfVlan(testCases[i].ifname, testCases[i].vf_num, testCases[i].vlan_id);
+       if (rc != testCases[i].rc) {
+           return -1;
+       }
+    }
+
+    return 0;
+}
+
+static int
+testVirNetDevSetVfConfig(const void *opaque G_GNUC_UNUSED)
+{
+    struct testCase {
+        const char *ifname;
+        const int rc;
+    };
+    int rc = 0;
+    size_t i = 0;
+    /* Nested functions are mocked so dummy values are used. */
+    const virMacAddr mac = { .addr = { 0xDE, 0xAD, 0xBE, 0xEF, 0xCA, 0xFE }};
+    const int vfNum = 1;
+    const int vlanid = 0;
+    bool *allowRetry = NULL;
+
+    const struct testCase testCases[] = {
+        { .ifname = "fakeiface-macerror", .rc = -EBUSY },
+        { .ifname = "fakeiface-altmacerror", .rc = -EINVAL },
+        { .ifname = "fakeiface-macerror-novlanerror", .rc = -EAGAIN },
+        { .ifname = "fakeiface-macerror-vlanerror", .rc = -ENODEV },
+        { .ifname = "fakeiface-nomacerror-novlanerror", .rc = 0 },
+    };
+
+    for (i = 0; i < sizeof(testCases) / sizeof(struct testCase); ++i) {
+       rc = virNetDevSetVfConfig(testCases[i].ifname, vfNum, &mac, vlanid, allowRetry);
+       if (rc != testCases[i].rc) {
+           return -1;
+       }
+    }
+    return 0;
+}
+
 static int
 mymain(void)
 {
@@ -76,6 +287,15 @@ mymain(void)
     DO_TEST_LINK("lo", VIR_NETDEV_IF_STATE_UNKNOWN, 0);
     DO_TEST_LINK("eth0-broken", VIR_NETDEV_IF_STATE_DOWN, 0);
 
+    if (virTestRun("Set VF MAC", testVirNetDevSetVfMac, NULL) < 0)
+        ret = -1;
+    if (virTestRun("Set VF MAC: missing MAC pointer", testVirNetDevSetVfMissingMac, NULL) < 0)
+        ret = -1;
+    if (virTestRun("Set VF VLAN", testVirNetDevSetVfVlan, NULL) < 0)
+        ret = -1;
+    if (virTestRun("Set VF Config", testVirNetDevSetVfConfig, NULL) < 0)
+        ret = -1;
+
     return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
 
-- 
2.32.0





More information about the libvir-list mailing list