Set VF MAC and VLAN ID in two different operations

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@canonical.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

src/libvirt_private.syms

@@ -2847,6 +2847,13 @@ virNetDevOpenvswitchSetTimeout;
 virNetDevOpenvswitchUpdateVlan;
 
 
+# util/virnetdevpriv.h
+virNetDevSendVfSetLinkRequest;
+virNetDevSetVfConfig;
+virNetDevSetVfMac;
+virNetDevSetVfVlan;
+
+
 # util/virnetdevtap.h
 virNetDevTapAttachBridge;
 virNetDevTapCreate;

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"
@@ -1509,16 +1511,15 @@ static struct nla_policy ifla_vfstats_policy[IFLA_VF_STATS_MAX+1] = {
     [IFLA_VF_STATS_MULTICAST]   = { .type = NLA_U64 },
 };
 
-
+int
-static int
+virNetDevSendVfSetLinkRequest(const char *ifname,
-virNetDevSetVfConfig(const char *ifname, int vf,
+                              int vfInfoType,
-                     const virMacAddr *macaddr, int vlanid,
+                              const void *payload,
-                     bool *allowRetry)
+                              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;
@@ -1527,9 +1528,6 @@ virNetDevSetVfConfig(const char *ifname, int vf,
         .ifi_index  = -1,
     };
 
-    if (!macaddr && vlanid < 0)
-        return -1;
-
     nl_msg = virNetlinkMsgNew(RTM_SETLINK, NLM_F_REQUEST);
 
     if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
@@ -1539,37 +1537,14 @@ virNetDevSetVfConfig(const char *ifname, int vf,
         nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
         goto buffer_too_small;
 
-
     if (!(vfinfolist = nla_nest_start(nl_msg, IFLA_VFINFO_LIST)))
         goto buffer_too_small;
 
     if (!(vfinfo = nla_nest_start(nl_msg, IFLA_VF_INFO)))
         goto buffer_too_small;
 
-    if (macaddr) {
+    if (nla_put(nl_msg, vfInfoType, payloadLen, payload) < 0)
-        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;
-    }
 
     nla_nest_end(nl_msg, vfinfo);
     nla_nest_end(nl_msg, vfinfolist);
@@ -1586,44 +1561,16 @@ virNetDevSetVfConfig(const char *ifname, int vf,
         err = (struct nlmsgerr *)NLMSG_DATA(resp);
         if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
             goto malformed_resp;
-
+        rc = err->error;
-        /* 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:
+        rc = 0;
         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;
 
@@ -1638,6 +1585,96 @@ virNetDevSetVfConfig(const char *ifname, int vf,
     goto cleanup;
 }
 
+int
+virNetDevSetVfVlan(const char *ifname,
+                   int vf,
+                   int vlanid)
+{
+    int ret = -1;
+    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)) {
+        virReportError(ERANGE, _("vlanid out of range: %d"), vlanid);
+        return -ERANGE;
+    }
+
+    ret = virNetDevSendVfSetLinkRequest(ifname, IFLA_VF_VLAN,
+                                        &ifla_vf_vlan, sizeof(ifla_vf_vlan));
+
+    if (ret < 0) {
+        virReportSystemError(-ret,
+                             _("Cannot set interface vlanid to %d for ifname %s vf %d"),
+                             vlanid, ifname ? ifname : "(unspecified)", vf);
+    }
+
+    VIR_DEBUG("RTM_SETLINK %s vf %d vlanid=%d - %s",
+              ifname, vf, vlanid, ret < 0 ? "Fail" : "Success");
+    return ret;
+}
+
+int
+virNetDevSetVfMac(const char *ifname, int vf,
+                  const virMacAddr *macaddr,
+                  bool *allowRetry)
+{
+    int ret = -1;
+    char macstr[VIR_MAC_STRING_BUFLEN];
+    struct ifla_vf_mac ifla_vf_mac = {
+        .vf = vf,
+        .mac = { 0, },
+    };
+
+    if (macaddr == NULL || allowRetry == NULL) {
+        virReportError(EINVAL, "%s", _("Invalid parameters: %d"));
+        return -EINVAL;
+    }
+
+    virMacAddrGetRaw(macaddr, ifla_vf_mac.mac);
+
+    ret = virNetDevSendVfSetLinkRequest(ifname, IFLA_VF_MAC,
+                                        &ifla_vf_mac, sizeof(ifla_vf_mac));
+    if (ret == -EINVAL && *allowRetry && !virMacAddrCmp(macaddr, &zeroMAC)) {
+        /* if allowRetry is true and the error was EINVAL, then
+         * silently return a failure so the caller can retry with a
+         * different MAC address. */
+    } else if (ret < 0) {
+        /* other errors are permanent */
+        virReportSystemError(-ret,
+                             _("Cannot set interface MAC to %s for ifname %s vf %d"),
+                             macaddr ? virMacAddrFormat(macaddr, macstr) : "(unchanged)",
+                             ifname ? ifname : "(unspecified)",
+                             vf);
+        *allowRetry = false; /* don't use retrying */
+    }
+
+    VIR_DEBUG("RTM_SETLINK %s vf %d MAC=%s - %s",
+              ifname, vf,
+              macaddr ? virMacAddrFormat(macaddr, macstr) : "(unchanged)",
+              ret < 0 ? "Fail" : "Success");
+    return ret;
+}
+
+int
+virNetDevSetVfConfig(const char *ifname,
+                     int vf,
+                     const virMacAddr *macaddr,
+                     int vlanid,
+                     bool *allowRetry)
+{
+    int ret = -1;
+
+    if ((ret = virNetDevSetVfMac(ifname, vf, macaddr, allowRetry)) < 0)
+        return ret;
+    if ((ret = virNetDevSetVfVlan(ifname, vf, vlanid)) < 0)
+        return ret;
+    return ret;
+}
+
 /**
  * virNetDevParseVfInfo:
  * Get the VF interface information from kernel by netlink, To make netlink
@@ -2391,6 +2428,50 @@ virNetDevVFInterfaceStats(virPCIDeviceAddress *vfAddr G_GNUC_UNUSED,
     return -1;
 }
 
+int
+virNetDevSendVfSetLinkRequest(const char *ifname G_GNUC_UNUSED,
+                              int vfInfoType G_GNUC_UNUSED,
+                              const void *payload G_GNUC_UNUSED,
+                              const size_t payloadLen G_GNUC_UNUSED)
+{
+    virReportSystemError(ENOSYS, "%s",
+                         _("Unable to send a VF SETLINK request on this platform"));
+    return -ENOSYS;
+}
+
+int
+virNetDevSetVfVlan(const char *ifname G_GNUC_UNUSED,
+                   int vf G_GNUC_UNUSED,
+                   int vlanid G_GNUC_UNUSED)
+{
+    virReportSystemError(ENOSYS, "%s",
+                         _("Unable to set a VF VLAN on this platform"));
+    return -ENOSYS;
+}
+
+int
+virNetDevSetVfMac(const char *ifname G_GNUC_UNUSED,
+                  int vf G_GNUC_UNUSED,
+                  const virMacAddr *macaddr G_GNUC_UNUSED,
+                  bool *allowRetry G_GNUC_UNUSED)
+{
+    virReportSystemError(ENOSYS, "%s",
+                         _("Unable to set a VF MAC on this platform"));
+    return -ENOSYS;
+}
+
+int
+virNetDevSetVfConfig(const char *ifname G_GNUC_UNUSED,
+                     int vf G_GNUC_UNUSED,
+                     const virMacAddr *macaddr G_GNUC_UNUSED,
+                     int vlanid G_GNUC_UNUSED,
+                     bool *allowRetry G_GNUC_UNUSED)
+{
+    virReportSystemError(ENOSYS, "%s",
+                         _("Unable to set a VF config on this platform"));
+    return -ENOSYS;
+}
+
 
 #endif /* defined(WITH_LIBNL) */
 

src/util/virnetdevpriv.h

@@ -0,0 +1,51 @@
+/*
+ * 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);

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,227 @@ testVirNetDevGetLinkInfo(const void *opaque)
     return 0;
 }
 
+# if defined(WITH_LIBNL)
+
+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;
+}
+
+# endif /* defined(WITH_LIBNL) */
+
 static int
 mymain(void)
 {
@@ -76,6 +303,19 @@ mymain(void)
     DO_TEST_LINK("lo", VIR_NETDEV_IF_STATE_UNKNOWN, 0);
     DO_TEST_LINK("eth0-broken", VIR_NETDEV_IF_STATE_DOWN, 0);
 
+# if defined(WITH_LIBNL)
+
+    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;
+
+# endif /* defined(WITH_LIBNL) */
+
     return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }