[libvirt] [Fwd: [PATCH v9] add 802.1Qbh and 802.1Qbg handling]

[Arnd Bergmann]

On Thursday 27 May 2010, Stefan Berger wrote:


> +static int
> +getPortProfileStatus(struct nlattr **tb, int32_t vf, uint16_t *status)
> +{
> +    int rc = 1;
> +    const char *msg = NULL;
> +    struct nlattr *tb2[IFLA_VF_PORT_MAX + 1],
> +                  *tb3[IFLA_PORT_MAX+1];
> +
> +    if (vf == PORT_SELF_VF) {
> +        if (tb[IFLA_PORT_SELF]) {
> +            if (nla_parse_nested(tb3, IFLA_PORT_MAX, tb[IFLA_PORT_SELF],
> +                                 ifla_port_policy)) {
> +                msg = _("error parsing nested IFLA_VF_PORT part");
> +                goto err_exit;
> +            }
> +        }
> +    } else {
> +        if (tb[IFLA_VF_PORTS]) {
> +            if (nla_parse_nested(tb2, IFLA_VF_PORT_MAX, tb[IFLA_VF_PORTS],
> +                                 ifla_vf_ports_policy)) {
> +                msg = _("error parsing nested IFLA_VF_PORTS part");
> +                goto err_exit;
> +            }
> +            if (tb2[IFLA_VF_PORT]) {
> +                if (nla_parse_nested(tb3, IFLA_PORT_MAX, tb2[IFLA_VF_PORT],
> +                                     ifla_port_policy)) {
> +                    msg = _("error parsing nested IFLA_VF_PORT part");
> +                    goto err_exit;
> +                }
> +            }
> +        }
> +    }

There may be multiple IFLA_VF_PORT attributes in the IFLA_VF_PORTS list,
so you cannot do nla_parse_nested. I think this should be nla_for_each_attr
instead, and compare the uuid to the one you expect.

> +        memcpy(ifla_vf_mac.mac, macaddr, 6);
> +
> +        rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_VFINFO_LIST,
> +                           NULL, 0);
> +        if (!rta ||
> +            !(vfinfolist = nlAppend(nlm, sizeof(nlmsgbuf),
> +                                    rtattbuf, rta->rta_len)))
> +            goto buffer_too_small;
> +
> +        rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_VF_INFO,
> +                           NULL, 0);
> +        if (!rta ||
> +            !(vfinfo = nlAppend(nlm, sizeof(nlmsgbuf),
> +                                rtattbuf, rta->rta_len)))
> +            goto buffer_too_small;
> +
> +        rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_VF_MAC,
> +                           &ifla_vf_mac, sizeof(ifla_vf_mac));
> +        if (!rta || !nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
> +            goto buffer_too_small;
> +
> +        rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_VF_VLAN,
> +                           &ifla_vf_vlan, sizeof(ifla_vf_vlan));
> +
> +        if (!rta || !nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
> +            goto buffer_too_small;
> +
> +        vfinfo->rta_len = (char *)nlm + nlm->nlmsg_len - (char *)vfinfo;
> +
> +        vfinfolist->rta_len = (char *)nlm + nlm->nlmsg_len -
> +                              (char *)vfinfolist;
> +    }

This part looks good now.

> +    if (vf == PORT_SELF_VF) {
> +        rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_PORT_SELF, NULL, 0);
> +    } else {
> +        rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_VF_PORTS, NULL, 0);
> +        if (!rta ||
> +            !(vfports = nlAppend(nlm, sizeof(nlmsgbuf),
> +                                 rtattbuf, rta->rta_len)))
> +            goto buffer_too_small;
> +
> +        /* begin nesting vfports */
> +        rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_VF_PORT, NULL, 0);
> +    }

But this still goes down the IFLA_PORT_SELF route because you pass PORT_SELF_VF
even for nltarget_kernel==false, where it makes no sense.
Maybe make the above

	if (vf == PORT_SELF_VF && nltarget_kernel)

> +    if (vf != PORT_SELF_VF) {
> +        /* end nesting of vfports */
> +        vfports->rta_len = (char *)nlm + nlm->nlmsg_len - (char *)vfports;
> +    }

Here too.

> +    if (nltarget_kernel) {
> +        if (nlComm(nlm, &recvbuf, &recvbuflen) < 0)
> +            return -1;
> +    } else {
> +        if (nlCommWaitSuccess(nlm, RTMGRP_LINK, &recvbuf, &recvbuflen,
> +                              5 * MICROSEC_PER_SEC) < 0)
> +            return -1;
> +    }

I don't understand this part yet. Do we need this difference?

> +    while (--repeats >= 0) {
> +        rc = link_dump(nltarget_kernel, NULL, ifindex, tb, &recvbuf);
> +        if (rc)
> +            goto err_exit;
> +        rc = getPortProfileStatus(tb, vf, &status);
> +        if (rc == 0) {
> +            if (status == PORT_PROFILE_RESPONSE_SUCCESS ||
> +                status == PORT_VDP_RESPONSE_SUCCESS) {
> +                break;
> +            } else if (status == PORT_PROFILE_RESPONSE_INPROGRESS) {
> +                // keep trying...
> +            } else {
> +                virReportSystemError(EINVAL,
> +                    _("error %d during port-profile setlink on ifindex %d"),
> +                    status, ifindex);
> +                rc = 1;
> +                break;
> +            }

Hmm, we seem to be missing an INPROGRESS status for Qbg. Any suggestions
what we should return there? Should we possibly just leave out
IFLA_PORT_RESPONSE in order to signal INPROGRESS, as in not clear yet?

> +    rc = doPortProfileOpCommon(nltarget_kernel,
> +                               physdev_ifname, physdev_ifindex,
> +                               macaddr,
> +                               vlanid,
> +                               NULL,
> +                               &portVsi,
> +                               virtPort->u.virtPort8021Qbg.instanceID,
> +                               NULL,
> +                               PORT_SELF_VF,
> +                               op);

This is where we pass PORT_SELF_VF together with nltarget_kernel=false,
as mentioned above.

	Arnd