On 2/1/22 09:28, 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 | 226 ++++++++++++++++++++++++++------------ src/util/virnetdevpriv.h | 44 ++++++++ tests/virnetdevtest.c | 230 ++++++++++++++++++++++++++++++++++++++- 4 files changed, 434 insertions(+), 73 deletions(-) create mode 100644 src/util/virnetdevpriv.h
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bc6fa191bf..e6493c2176 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2847,6 +2847,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 d93b2c6a83..043225d31f 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" @@ -1509,16 +1511,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)
Here and everywhere else, please put one argument per line.
{ - 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; @@ -1526,9 +1524,11 @@ virNetDevSetVfConfig(const char *ifname, int vf, .ifi_family = AF_UNSPEC, .ifi_index = -1, }; + int rc = 0;
We usually initialize return values to -1 so that they don't have to be overwritten in each error path.
- if (!macaddr && vlanid < 0) + if (payload == NULL || payloadLen == 0) { return -1; + }
This seems rather excessive. I know you just copied whatever was in the original code, but neither of callers passes NULL/0.
nl_msg = virNetlinkMsgNew(RTM_SETLINK, NLM_F_REQUEST);
@@ -1546,30 +1546,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); @@ -1582,48 +1560,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;
No need for formatting change.
- - /* 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;
@@ -1638,6 +1588,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, + };
Alignment.
+ + /* VLAN ids 0 and 4095 are reserved per 802.1Q but are valid values. */ + if ((vlanid < 0 || vlanid > 4095)) { + return -ERANGE;
Here, we need to report an error because we do so in the other error path. The rule of thumb is that either all or none error paths report an error.
+ } + + 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"),
Pre-existing, but error message are exempt from 80-chars long line rule so that they can be easily 'git-grep'-ed. But since you're touching this code it's worth fixing.
+ vlanid, ifname ? ifname : "(unspecified)", vf); + rc = requestError;
So there's no difference between @rc and @requestError. They both have the same value in the end.
+ } + 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) +{
Basically all comments from above apply here too.
+ 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 @@ -2391,6 +2435,44 @@ 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 -1;
I'm inclined to return -ENOSYS rather than -1 because the real implementation would return a real -errno. I know it doesn't matter really because neither of callers looks at errno, they merely check for <0 but consistency is paramount. Thus if one variant of function returns -errno then the other should too.
+} + +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 -1; +} + +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 -1; +} + +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 -1; +} +
#endif /* defined(WITH_LIBNL) */
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..c8dba05c31 100644 --- a/tests/virnetdevtest.c +++ b/tests/virnetdevtest.c
Bonus points for extensive tests. Michal