On Tuesday 12 October 2010, John Fastabend wrote:
> On 9/28/2010 8:10 AM, Jens Osterkamp wrote:
> > This is the implementation of the edge control protocol (ECP) and VSI
> > discovery protocol (VDP) currently being specified in IEEE 802.1Qbg.
> >
> > This implementation extends the infrastructure defined lldpad to send and
> > receive ECP frames with a new (yet to be defined) ethertype.
> > Received frames are validated and analyzed before the content is handed to the
> > upper layer protocol (ULP, VDP in this case) for further processing. Frames
> > to be transmitted are compiled from VSI (guest interface) profiles registered
> > on a interface.
> > Reception and transmission of ECP frames is controlled by RX and TX state
> > machines, timeouts are handled timeout functions.
> > The patch still contains a lot of debug code to allow low-level protocol
> > analysis.
> >
> > VDP serves as the upper layer protocol (ULP) for TLVs communicated via the
> > ECP protocol.
> > For this it registers as a new module in lldpad. The VDP module supports a
> > station and a bridge role. As a station, new VSI (virtual station interface)
> > profiles can be registered to the VDP module using lldptool or libvirt.
> > These profiles are then announced to an adjacent switch. Transmitted profiles
> > are processed to the desired state by the VDP station state machine.
> > As a bridge, the VDP module waits for new profiles received in TLVs by ECP.
> > The received profiles are processed to the desired state by a VDP bridge
> > state machine.
> >
> > VDP module parameters are stored in the "vdp" section under the appropriate
> > interface.
> >
> > The patch still contains a lot of debug code to allow analysis of VDP
> > protocol behavior.
> >
> > Signed-off-by: Jens Osterkamp <jens@xxxxxxxxxxxxxxxxxx>
> > ---
> > Makefile.am | 10 +-
> > ecp/ecp.c | 79 ++++
> > ecp/ecp.h | 101 +++++
> > ecp/ecp_rx.c | 599 ++++++++++++++++++++++++++
> > ecp/ecp_tx.c | 494 ++++++++++++++++++++++
> > include/lldp.h | 1 +
> > include/lldp_evb.h | 6 +
> > include/lldp_vdp.h | 159 +++++++
> > lldp/l2_packet.h | 2 +
> > lldp/ports.h | 11 +-
> > lldp_vdp.c | 1185 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > lldpad.c | 2 +
> > 12 files changed, 2642 insertions(+), 7 deletions(-)
> > create mode 100644 ecp/ecp.c
> > create mode 100644 ecp/ecp.h
> > create mode 100644 ecp/ecp_rx.c
> > create mode 100644 ecp/ecp_tx.c
> > create mode 100644 include/lldp_vdp.h
> > create mode 100644 lldp_vdp.c
> >
[snip]
> > + return false;
> > + case ECP_RX_RECEIVE_ECPDU:
> > + if (vd->ecp.seqECPDU == vd->ecp.lastSequence) {
> > + printf("%s(%i):-(%s) seqECPDU %x, lastSequence %x\n", __func__, __LINE__,
> > + vd->ifname, vd->ecp.seqECPDU, vd->ecp.lastSequence);
> > + ecp_rx_change_state(vd, ECP_RX_RESEND_ACK);
> > + return true;
> > + }
> > + if (vd->ecp.seqECPDU != vd->ecp.lastSequence) {
> > + ecp_rx_change_state(vd, ECP_RX_RESEND_ACK);
> > + return true;
> > + }
> > + return false;
> > + case ECP_RX_SEND_ACK:
>
> How can we get to this state? Did I miss something or could the ECP_RX_SEND_ACK and ECP_RX_RESEND_ACK states be combined or ECP_RX_SEND_ACK removed outright?
Yep, can be combined but not entirely removed because different actions need to be taken for both states according to draft. Fixed in my next series.
>
> > + ecp_rx_change_state(vd, ECP_RX_RECEIVE_WAIT);
> > + return false;
> > + case ECP_RX_RESEND_ACK:
> > + ecp_rx_change_state(vd, ECP_RX_RECEIVE_WAIT);
> > + return false;
> > + default:
> > + printf("ERROR: The ECP_RX State Machine is broken!\n");
> > + log_message(MSG_ERR_RX_SM_INVALID, "%s", vd->ifname);
> > + return false;
[snip]
> > + if ((ptlv->size+fb_offset) > ETH_MAX_DATA_LEN)
> > + goto error;
> > + memcpy(vd->ecp.tx.frameout+fb_offset,
> > + ptlv->tlv, ptlv->size);
> > + datasize += ptlv->size;
> > + fb_offset += ptlv->size;
>
> I think the ptlv needs to be free'd here? Else where is it getting free'd.
Fixed in my next series.
>
> > + }
> > + }
> > +
> > + /* The End TLV marks the end of the LLDP PDU */
> > + ptlv = pack_end_tlv();
> > + if (!ptlv || ((ptlv->size + fb_offset) > ETH_MAX_DATA_LEN))
> > + goto error;
> > + memcpy(vd->ecp.tx.frameout + fb_offset, ptlv->tlv, ptlv->size);
> > + datasize += ptlv->size;
> > + fb_offset += ptlv->size;
> > + ptlv = free_pkd_tlv(ptlv);
> > +
> > + if (datasize > ETH_MAX_DATA_LEN)
[snip]
> > +
> > + memset(vdp, 0, sizeof(struct tlv_info_vdp));
> > + memcpy(vdp, tlv->info, tlv->length);
> > +
> > + if (vdp_validate_tlv(vdp)) {
> > + printf("%s(%i): Invalid TLV received !\n", __func__, __LINE__);
> > + goto out_err;
>
> Should be goto out_vdp
Fixed.
>
> > + }
> > +
> > + profile = malloc(sizeof(struct vsi_profile));
> > +
> > + if (!profile) {
> > + printf("%s(%i): unable to allocate profile !\n", __func__, __LINE__);
> > + goto out_vdp;
> > + }
> > +
> > + memset(profile, 0, sizeof(struct vsi_profile));
> > +
> > + profile->mode = vdp->mode;
> > + profile->response = vdp->response;
> > +
> > + profile->mgrid = vdp->mgrid;
> > + profile->id = ntoh24(vdp->id);
> > + profile->version = vdp->version;
> > + memcpy(&profile->instance, &vdp->instance, 16);
> > + memcpy(&profile->mac, &vdp->mac_vlan.mac, MAC_ADDR_LEN);
> > + profile->vlan = ntohs(vdp->mac_vlan.vlan);
> > +
> > + profile->port = port;
> > +
> > + if (vd->role == VDP_ROLE_STATION) {
> > + /* do we have the profile already ? */
> > + LIST_FOREACH(p, &vd->profile_head, profile) {
> > + if (vdp_profile_equal(p, profile)) {
> > + printf("%s(%i): station: profile found, localChange %i ackReceived %i!\n",
> > + __func__, __LINE__, p->localChange, p->ackReceived);
> > +
> > + p->ackReceived = true;
> > +
> > + vdp_vsi_sm_station(p);
> > + } else {
> > + printf("%s(%i): station: profile not found !\n", __func__, __LINE__);
> > + /* ignore profile */
> > + }
> > + }
> > + }
> > +
> > + if (vd->role == VDP_ROLE_BRIDGE) {
> > + /* do we have the profile already ? */
> > + LIST_FOREACH(p, &vd->profile_head, profile) {
> > + if (vdp_profile_equal(p, profile)) {
> > + break;
> > + }
> > + }
> > +
> > + if (p) {
> > + printf("%s(%i): bridge: profile found !\n", __func__, __LINE__);
> > + } else {
> > + printf("%s(%i): bridge: profile not found !\n", __func__, __LINE__);
> > + /* put it in the list */
> > + profile->state = VSI_UNASSOCIATED;
> > + LIST_INSERT_HEAD(&vd->profile_head, profile, profile );
> > + }
> > +
> > + vdp_vsi_sm_bridge(profile);
> > + }
> > +
>
>
> Here the profile is added to a list if the port is in the VDP_ROLE_BRIDGE mode. Otherwise the profile is only used to lookup an existing profile? Looks like there might be a memory leak if the profile already exists. Does the profile need to be cleaned up the somewhere?
The standard is not clear on what to with profiles that e.g. have been deassociated or have reached VSI_EXIT. In my next series I have added code to remove the profile in the VSI_EXIT case.
>
> > + return 0;
> > +
> > +out_vdp:
> > + free(vdp);
> > +out_err:
> > + printf("%s(%i): error !\n", __func__, __LINE__);
> > + return 1;
> > +
> > +}
>
>
> The rest looks good.
Thanks ! I will post a new series soon.
Jens
--
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/virtualization
