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.shcherba...@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
