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>
---
 src/libvirt_private.syms |   7 ++
 src/util/virnetdev.c     | 189 ++++++++++++++++++++-------------
 src/util/virnetdevpriv.h |  44 ++++++++
 tests/virnetdevtest.c    | 222 ++++++++++++++++++++++++++++++++++++++-
 4 files changed, 389 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..dfd4506c21 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,101 @@ 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;
+    } else 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

On Thu, Nov 18, 2021 at 11:43:24AM +0300, Dmitrii Shcherbakov wrote:
> 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>
> ---
>  src/libvirt_private.syms |   7 ++
>  src/util/virnetdev.c     | 189 ++++++++++++++++++++-------------
>  src/util/virnetdevpriv.h |  44 ++++++++
>  tests/virnetdevtest.c    | 222 ++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 389 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..dfd4506c21 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,101 @@ 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;
> +    } else if ((rc = virNetDevSetVfVlan(ifname, vf, vlanid)) < 0) {
> +        return rc;
> +    }

Minor point I would get rid of the 'else' here, to make it obvious that
in the "success" case, we're intending to be making both of these method
calls.

    if ((rc = virNetDevSetVfMac(ifname, vf, macaddr, allowRetry)) < 0)
        return rc;
    if ((rc = virNetDevSetVfVlan(ifname, vf, vlanid)) < 0)
        return rc;

Or alternatively compress them


    if ((rc = virNetDevSetVfMac(ifname, vf, macaddr, allowRetry)) < 0 ||
        (rc = virNetDevSetVfVlan(ifname, vf, vlanid)) < 0)
        return rc;



> +    return rc;
> +}
> +


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Hi Daniel,

Addressed the below in v6.

https://listman.redhat.com/archives/libvir-list/2021-November/msg00539.html

> > +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;
> > +    } else if ((rc = virNetDevSetVfVlan(ifname, vf, vlanid)) < 0) {
> > +        return rc;
> > +    }
>
> Minor point I would get rid of the 'else' here, to make it obvious that
> in the "success" case, we're intending to be making both of these method
> calls.
>
>     if ((rc = virNetDevSetVfMac(ifname, vf, macaddr, allowRetry)) < 0)
>         return rc;
>     if ((rc = virNetDevSetVfVlan(ifname, vf, vlanid)) < 0)
>         return rc;
>
> Or alternatively compress them
>
>
>     if ((rc = virNetDevSetVfMac(ifname, vf, macaddr, allowRetry)) < 0 ||
>         (rc = virNetDevSetVfVlan(ifname, vf, vlanid)) < 0)
>         return rc;
>
>
>
> > +    return rc;
> > +}
> > +


Best Regards,
Dmitrii Shcherbakov
LP/oftc: dmitriis