[lldp-devel] [PATCH 1/3] vdp22 filter interface support

Thomas-Mich Richter tmricht at linux.vnet.ibm.com
Tue Nov 26 09:23:28 UTC 2013


On 11/25/2013 10:55 PM, John Fastabend wrote:
> On 11/21/2013 01:26 AM, Thomas Richter wrote:
>> The VDP22 protocol allows the return of changed filter
>> information data from the switch to the originator. This
>> requires a change in the netlink message format. Now lldpad
>> support 2 netlink message formats:
>> 1. The draft 0.2 version. In this version the filter information
>>     data is received but not returned to the originator.
>>
>>     The receive message format is:
>>     IFLA_IFNAME
>>     IFLA_VFINFO_LIST
>>          IFLA_VF_INFO
>>          IFLA_VF_MAC
>>          IFLA_VF_VLAN
>>     end
>>     IFLA_VF_PORTS
>>          IFLA_VF_PORT
>>          IFLA_PORT_VSI_TYPE
>>          IFLA_PORT_REQUEST
>>          IFLA_PORT_INSTANCE_UUID
>>     end
>>
>>     The returned message format sent as reply from lldpad to the
>>     originator is:
>>     IFLA_IFNAME
>>     IFLA_VF_PORTS
>>          IFLA_VF_PORT
>>          IFLA_PORT_VF
>>          IFLA_PORT_INSTANCE_UUID
>>          IFLA_PORT_RESPONSE
>>     end
>>
>> 2. The ratified standard version VDP22 has the following
>>     netlink message format:
>>     IFLA_IFNAME
>>     IFLA_VF_PORTS
>>          IFLA_VF_PORT
>>          IFLA_PORT_VSI_TYPE22
>>          IFLA_PORT_VSI_FILTER
>>          IFLA_PORT_REQUEST
>>          IFLA_PORT_INSTANCE_UUID
>>     end
>>
> 
> [...]
> 
>> +/*
>> + * New version, implemented as library function and header file in lldpad-devel
>> + * package.
>> + *
>> + * Netlink message for QBG 2.2 ratified standard
>> + */
>> +
>> +enum {                    /* 802.1Qbg VDP ratified standard */
>> +    IFLA_PORT_VSI_TYPE22 = IFLA_PORT_MAX,
>> +    IFLA_PORT_VSI_FILTER,
>> +    __IFLA_PORT_MAX_NEW
>> +};
>> +
> 
> The problem with pushing the defines to the end like this is you get a
> dependency between lldpad and the application pushing the message to
> it. For example if you build both libvirt and lldpad at some point in
> time then upgrade lldpad or libvirt they may no longer be in sync and
> these messages will fail.
> 
> We've been going down this path of using the kernel API because it
> existed and we presumed at some point there would be a kernel consumer
> for this. As it is not a single driver has implemented this API as best
> I can tell.
> 
> At this point I'm trying to decide how to handle this I really don't
> like creating dependencies between separate applications like this.
> Any thoughts/ideas? I see you submitted a kernel patch which would
> I think would resolve this by making these APIs part of the kernel
> kABI. Once Dave Miller opens the net-next merge window again maybe
> we need to revisit that patch.
> 
> 
John,

I understand your concerns. You might have noticed:
I tried to submit a patch to update if_link.h file to include the new
structures and attributes IFLA_PORT_VSI_TYPE22 and IFLA_PORT_VSI_FILTER.
Dave Miller turned it down, he does not accept a define without usage.
I can follow him (a bit, after all its just a structure definition)
and that is the reason why I did not push it further.
IFLA_PORT_VSI_TYPE is not used either and he might decide to drop it
from the kernel if_link.h and then libvirtd vs lldpad communication
is broken.

However as the code stands now, the only user of the new defines for the
attributes IFLA_PORT_VSI_TYPE22 and IFLA_PORT_VSI_FILTER is my test
program vdptest.c. Libvirtd is not updated to use VDP22 protocols (at the
moment, that will come later).
And both programs vdptest and lldpad will be built together, different 
builds of lldpad and libvirtd does not cause problems. 
Libvirtd does not use the new
IFLA_PORT_VSI_TYPE22 and IFLA_PORT_VSI_FILTER attributes. Libvirtd
uses IFLA_VFINFO_LIST to transfer the mac/vlan information. (This design
decision was not mine, it was done before I joint this team).

My ideas:
1. Try my patch again. But I am unsure if we succeed. In fact Qbg is
   not very active (dead might be a better word). And why should kernel
   define something it does not use. This is a very strong point.
   And we do not know any hardware which supports Qbg22 to actually know
   if we have done it right.
2. I work on a command line interface for vdp22. It idea is to use the
   lldptool to trigger commands to associate/de-associate VSIs. Once this
   works the customers (libvirtd, vdptest) could use this interface defined
   by lldpad. This would eliminate the external dependency on the kernel.
3. Use the current libvirtd scheme of IFLA_VFINFO_LIST and extend it with
   a suitable structure to pass the 4 byte group id. For example (mis)use 
   attribute ifla_vf_tx_rate. BTW we have discussed this before.
4. Or we could use the ifla_vf_mac, which contains space for 32 byte mac 
   address and use the remaining bytes to store the group id. This is a
   real hack. 

I will work on the remaining findings and will submit my patch again.
As said before, this gives me a chance to test it (in fact I have another
big patch waiting for you: ~100 automated test cases for evb22, ecp22, vdp22).
It should not break the currently released libvirtd/lldpad communication. 


>> +/*
>> + * Parse the IFLA_VF_PORT block of the netlink message.
>>    * Return zero on success and errno else.
>> + *
>> + * Code to parse netlink message format 1. May contain IFLA_PORT_VSI_TYPE22
>> + * and IFLA_PORT_VSI_FILTER attributes which make it netlink message format 2.
> 
> If it may contain a IFLA_PORT_VSI_TYPE22 type why is it not in the
> nla_policy for pc_vfport?
> 
>>    */
>> -static int vdpnl_vfports(struct nlattr *vfports, struct vdpnl_vsi *vsi)
>> +static struct nla_policy  pc_vfport[IFLA_MAX + 1] = {
>> +    [IFLA_PORT_VF] = {
>> +                .type = NLA_U32
>> +            },
>> +    [IFLA_PORT_VSI_TYPE] = {
>> +                .minlen = sizeof(struct ifla_port_vsi)
>> +            },
> 
> I expected to see another entry for type22 here like this,
> 
>     [IFLA_PORT_VSI_TYPE22] = { ... },
> 
>> +    [IFLA_PORT_INSTANCE_UUID] = {
>> +                .minlen = PORT_UUID_MAX
> 
> should this be '.len = PORT_UUID_MAX' I think that this is a max length
> not a minimum length?
> 
>> +            },
>> +    [IFLA_PORT_HOST_UUID] = {
>> +                .minlen = PORT_UUID_MAX
> 
> Same here max?
> 
>> +            },
>> +    [IFLA_PORT_REQUEST] = {
>> +                .type = NLA_U8
>> +            },
> 
> Also same question for IFLA_PORT_VSI_FILTER?
> 
>> +};
>> +
>> +static int parse_vf_port(struct vdpnl_vsi *vsi, struct nlattr *tb)
>>   {
>>       char instance[VDP_UUID_STRLEN + 2];
>> -    struct nlattr *tb_vf_ports, *tb3[IFLA_PORT_MAX + 1];
>> -    int rem;
>> +    struct nlattr *vf[IFLA_PORT_MAX + 1];
>> +    size_t dim = sizeof(vf) / sizeof(vf[0]);
>> +    int rc;
>>
>> -    if (!vfports) {
>> -        LLDPAD_DBG("%s:FOUND NO IFLA_VF_PORTS\n", __func__);
>> +    memset(vf, 0, sizeof(vf));
>> +    rc = nla_parse(vf, dim, nla_data(tb), nla_len(tb), pc_vfport);
> 
> Because you don't have the IFLA_PORT_VSI_FILTER and IFLA_PORT_VSI_TYPE22
> in the policy these attributes are not validated.
> 
> 
>> +    if (rc) {
>> +        LLDPAD_ERR("%s:IFLA_VF_PORT parsing error\n", __func__);
>>           return -EINVAL;
>>       }
>> -
>> -    nla_for_each_nested(tb_vf_ports, vfports, rem) {
>> -        if (nla_type(tb_vf_ports) != IFLA_VF_PORT) {
>> -            LLDPAD_DBG("%s:not a IFLA_VF_PORT skipping\n",
>> +    if (vf[IFLA_PORT_VF])
>> +        vsi->vf = nla_get_u32(vf[IFLA_PORT_VF]);
>> +    if (vf[IFLA_PORT_HOST_UUID]) {
>> +        unsigned char *uuid;
>> +
>> +        uuid = (unsigned char *)nla_data(vf[IFLA_PORT_HOST_UUID]);
>> +        vdp_uuid2str(uuid, instance, sizeof(instance));
>> +        LLDPAD_DBG("%s:IFLA_PORT_HOST_UUID:%s\n", __func__, instance);
>> +    }
>> +    if (vf[IFLA_PORT_RESPONSE]) {
>> +        vsi->response = nla_get_u16(vf[IFLA_PORT_RESPONSE]);
>> +        LLDPAD_DBG("%s:IFLA_PORT_RESPONSE:%d\n", __func__,
>> +                vsi->response);
>> +    }
>> +    if (vf[IFLA_PORT_VSI_TYPE]) {
>> +        struct ifla_port_vsi *p;
>> +
>> +        p = (struct ifla_port_vsi *)nla_data(vf[IFLA_PORT_VSI_TYPE]);
>> +        vsi->vsi_typeid = p->vsi_type_id[2] << 16
>> +                | p->vsi_type_id[1] << 8 | p->vsi_type_id[0];
>> +        vsi->vsi_mgrid = p->vsi_mgr_id;
>> +        vsi->vsi_typeversion = p->vsi_type_version;
>> +        vsi->nl_version = vdpnl_nlf1;
>> +        if (!vsi->maclist) {
>> +            LLDPAD_ERR("%s:VDP 0.2/2.2 filter error\n",
>>                      __func__);
>> -            continue;
>> +            return -EINVAL;
>>           }
>> -        if (nla_parse_nested(tb3, IFLA_PORT_MAX, tb_vf_ports,
>> -            ifla_port_policy)) {
>> -            LLDPAD_ERR("%s:IFLA_PORT_MAX parsing failed\n",
>> +    } else if(vf[IFLA_PORT_VSI_TYPE22] && vf[IFLA_PORT_VSI_FILTER]) {
>> +        struct ifla_port_vsi22 *p;
>> +
>> +        if (vsi->maclist) {
>> +            LLDPAD_ERR("%s:VDP 0.2/2.2 filter error\n",
>>                      __func__);
>>               return -EINVAL;
> 
> 
> [...]
> 
>> +        p = (struct ifla_port_vsi22 *)
>> +                    nla_data(vf[IFLA_PORT_VSI_TYPE22]);
>> +        vsi->vsi_typeid = p->vsi_type_id[2] << 16
>> +                | p->vsi_type_id[1] << 8 | p->vsi_type_id[0];
>> +        vsi->vsi_typeversion = p->vsi_type_version;
>> +        vsi->hints = p->vsi_hints;
>> +        vsi->filter_fmt = p->vsi_filter_fmt;
>> +        vsi->vsiid_fmt = p->vsi_uuidfmt;
>> +        memcpy(vsi->vsi_mgrid2, p->vsi_mgrid, PORT_UUID_MAX);
>> +        vsi->nl_version = vdpnl_nlf2;
>> +        vsi->macsz = p->vsi_filter_num;
>> +        vsi->maclist = calloc(vsi->macsz, sizeof(*vsi->maclist));
>> +        if (!vsi->maclist)
>> +            return -ENOMEM;
>> +        parse_filter_data(vsi, vf[IFLA_PORT_VSI_FILTER]);
>> +    } else {
>> +        LLDPAD_ERR("%s:IFLA_PORT_VSI_TYPE/FILTER missing\n", __func__);
>> +        return -EINVAL;
>> +    }
>> +    if (vf[IFLA_PORT_INSTANCE_UUID])
>> +        nla_memcpy(vsi->vsi_uuid, vf[IFLA_PORT_INSTANCE_UUID],
>> +               sizeof(vsi->vsi_uuid));
>> +    else {
>> +        LLDPAD_ERR("%s:IFLA_PORT_INSTANCE_UUID missing\n", __func__);
> 
> do you need to free vsi->maclist at this point?
> 
>> +        return -EINVAL;
>> +    }
>> +    if (vf[IFLA_PORT_REQUEST])
>> +        vsi->request = nla_get_u8(vf[IFLA_PORT_REQUEST]);
>> +    else {
> 
> same here free vsi->maclist?
> 
>> +        LLDPAD_ERR("%s:IFLA_PORT_REQUEST missing\n", __func__);
>> +        return -EINVAL;
>>       }
>>       return 0;
>>   }
>>
> 
> [...]
> 
>> +/*
>> + * Parse the IFLA_VF_INFO block of the netlink message.
>> + * Return zero on success and errno else.
>> + *
>> + * Code and parse netlink message format 1.
>> + */
>> +static struct nla_policy  pc_vlanmac[IFLA_MAX + 1] = {
>> +    [IFLA_VF_MAC] = {
>> +                .minlen = sizeof(struct ifla_vf_mac)
>> +            },
>> +    [IFLA_VF_VLAN] = {
>> +                .minlen = sizeof(struct ifla_vf_vlan)
>> +            }
>> +};
>>
>> -        if (vf[IFLA_VF_MAC]) {
>> -            struct ifla_vf_mac *mac = RTA_DATA(vf[IFLA_VF_MAC]);
>> +static int parse_vf_info(struct vdpnl_vsi *vsi, struct nlattr *tb)
>> +{
>> +    struct nlattr *vf[IFLA_VF_MAX + 1];
>> +    size_t dim = sizeof(vf) / sizeof(vf[0]);
>> +    int rc;
>> +    bool have_mac = false, have_vid = false;
>>
>> -            memcpy(vsi->maclist->mac, mac->mac, ETH_ALEN);
>> -            have_mac = true;
>> -        }
>> +    memset(vf, 0, sizeof(vf));
>> +    rc = nla_parse(vf, dim, nla_data(tb), nla_len(tb), pc_vlanmac);
>> +    if (rc || !vf[IFLA_VF_VLAN]) {
>> +        LLDPAD_ERR("%s:IFLA_VF_VLAN missing\n", __func__);
>> +        return -EINVAL;
>> +    }
>> +    vsi->nl_version = vdpnl_nlf1;
>> +    vsi->macsz = 1;
>> +    vsi->maclist = calloc(1, sizeof(*vsi->maclist));
>> +    if (!vsi->maclist)
>> +        return -ENOMEM;
>> +    if (vf[IFLA_VF_MAC]) {
>> +        struct ifla_vf_mac *mac = nla_data(vf[IFLA_VF_MAC]);
>> +
>> +        memcpy(vsi->maclist->mac, mac->mac, ETH_ALEN);
>> +        have_mac = true;
>> +    }
>>
>> -        if (vf[IFLA_VF_VLAN]) {
>> -            struct ifla_vf_vlan *vlan = RTA_DATA(vf[IFLA_VF_VLAN]);
>> +    if (vf[IFLA_VF_VLAN]) {
>> +        struct ifla_vf_vlan *vlan = nla_data(vf[IFLA_VF_VLAN]);
>>
>> -            vsi->maclist->vlan = vlan->vlan;
>> -            vsi->maclist->qos = vlan->qos;
>> +        vsi->maclist->vlan = vlan->vlan;
>> +        vsi->maclist->qos = vlan->qos;
>> +        if (vlan->vlan <= 4095 && vlan->qos <= 7)
>>               have_vid = true;
>> -        }
>> -        LLDPAD_DBG("%s:have_vid:%d have_mac:%d\n", __func__, have_vid,
>> +    }
>> +    LLDPAD_DBG("%s:have_vid:%d have_mac:%d\n", __func__, have_vid,
>>                  have_mac);
>> -        if (have_vid && have_mac)
>> -            vsi->filter_fmt = VDP22_FFMT_MACVID;
>> -        else if (have_vid)
>> -            vsi->filter_fmt = VDP22_FFMT_VID;
>> -        else
>> -            return -EINVAL;
>> +    if (have_vid && have_mac)
>> +        vsi->filter_fmt = VDP22_FFMT_MACVID;
>> +    else if (have_vid)
>> +        vsi->filter_fmt = VDP22_FFMT_VID;
>> +    else
> 
> free vsi->maclist?
> 
>> +        rc = -EINVAL;
>> +    return rc;
>> +}
>> +
> 
> [...]
>>   /*
>>    * Build the variable part of the netlink reply message for status inquiry.
>>    * It contains the UUID and the response field for the VSI profile.
>> + *
>> + * If the vlan number changed, return the new vlan/qos to the caller.
>>    */
>> -static void vdpnl_reply2(struct vdpnl_vsi *p, struct nlmsghdr *nlh)
>> +static void vdpnl_reply2(struct vdpnl_vsi *p, struct nl_msg *nlh)
>>   {
>>       char instance[VDP_UUID_STRLEN + 2];
>>
>> -    mynla_put(nlh, IFLA_PORT_INSTANCE_UUID, sizeof p->vsi_uuid,
>> +    nla_put(nlh, IFLA_PORT_INSTANCE_UUID, sizeof p->vsi_uuid,
>>             p->vsi_uuid);
>>       vdp_uuid2str(p->vsi_uuid, instance, sizeof instance);
>>       LLDPAD_DBG("%s:IFLA_PORT_INSTANCE_UUID:%s\n", __func__, instance);
>> -    mynla_put_u32(nlh, IFLA_PORT_VF, PORT_SELF_VF);
>> +    nla_put_u32(nlh, IFLA_PORT_VF, PORT_SELF_VF);
>>       LLDPAD_DBG("%s:IFLA_PORT_VF:%d\n", __func__,  PORT_SELF_VF);
>>       if (p->response != VDP_RESPONSE_NO_RESPONSE) {
>> -        mynla_put_u16(nlh, IFLA_PORT_RESPONSE, p->response);
>> +        nla_put_u16(nlh, IFLA_PORT_RESPONSE, p->response);
> 
> Also we should start checking the return values from nla_put_*. They
> can fail if the payload becomes bigger than the default message size.
> 
>>           LLDPAD_DBG("%s:IFLA_PORT_RESPONSE:%d\n", __func__,
>>                  p->response);
>> +        if (p->macsz && p->maclist->changed)
>> +            vdpnl_reply3(p, nlh);
>> +    }
>> +}
>> +
> 
> [...]
> 
> Besides the list of comments the big issue is the compatibilite with
> the kernel header files.
> 
> Thanks,
> John
> 
> 


-- 
Thomas Richter, Dept 3250, IBM LTC Boeblingen, Data Center Networking
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294



More information about the lldp-devel mailing list