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

[Stefan Berger]

On Thu, 2010-05-27 at 13:55 +0200, Arnd Bergmann wrote:
> 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.
> 

Ok. I'll change that for v10.

> > +        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)
> 

So I'll pass vf = 0 and be done with it? I don't find this check for
nltarget_kernel particularly intuitive. Why should the function create
different messages for a kernel driver versus a user space daemon? Is
this a technology specific thing that would prevent this type of message
from being sent. Obviously it does work for Scott's 802.1Qbh part.


> > +    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?

>From my experience with experiments that I have made I can only say that
we do need it. The sending part is a little different and the receiveing
part needs to filter out noise and only pick the response to the request
that was previously sent.

> 
> > +    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

You mean that's not defined in the (pre-)standard?

> what we should return there? Should we possibly just leave out
> IFLA_PORT_RESPONSE in order to signal INPROGRESS, as in not clear yet?

We want to be able to signal failure in case the switch setup failed so
that we don't have a malfunctioning network interface where the user
then doesn't know what to debug. In case of failure we simply wouldn't
start the VM or hotplug the interface and return an indication that
something went wrong during the negotiation with the switch.

> 
> > +    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.
> 

nltarget_kernel is in fact false here. So now if I change the
PORT_SELF_VF to '0', then I suppose it will be ok? The vf in the netlink
message to lldpad will then show 0 rather than PORT_SELF_VF (-1). I
guess 0 wouldn't have the meaning of the 0-st virtual function, but
counting would start at 1?

   Stefan

> 	Arnd