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@xxxxxxxxxxxxx>
> ---
> 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 :|
