[libvirt PATCH v4 1/3] 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(a)canonical.com&gt; --- 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