LLDP Layers hold redundant data

[Galadros]

As best I understand it, layers are supposed to be non-overlapping. By this I mean that fields of a given layer struct (in this case LinkLayerDiscovery) shouldn't contain any data that is also contained in other layer structs. As-is, the information in the LinkLayerDiscoveryInfo layer is entirely redundant with the Values field of LinkLayerDiscovery:

type LinkLayerDiscovery struct {
    BaseLayer
    ChassisID LLDPChassisID
    PortID    LLDPPortID
    TTL       uint16
    Values    []LinkLayerDiscoveryValue
}

All information that the LinkLayerDiscoveryInfo struct contains is parsed purely from the Values list in LinkLayerDiscovery in lines 846 to 883 of lldp.go. Even if I'm mistaken and layers can overlap in such a fashion, the way that these two layers are currently written makes it impossible to implement the DecodingLayer interface. Since information from the earlier layer is required to decode the LinkLayerDiscoveryInfo layer, I can't write a DecodeFromBytes method. (As-is, the DecodeLinkLayerDiscovery() function decodes into two different layers).

There are two different fixes that are possible. One would be to entirely merge the LinkLayerDiscoveryInfo layer into the LinkLayerDiscovery layer, since they're already redundant. The other would be to remove the Values field from the LinkLayerDiscovery struct (said field is never used except internally within DecodeLinkLayerDiscovery())and separate out the latter half of parsing done in DecodeLinkLayerDiscovery()into its own method to decode LinkLayerDiscoveryInfo. I plan to submit a pull request changing this as well as implementing the DecodingLayer interface for both of these layers, but I wanted to first check that I haven't misunderstood the requirements of what a "layer" is.

TD;DR: The LinkLayerDiscovery and LinkLayerDiscoveryInfo layers overlap and can't be turned into DecodingLayers; I plan to fix this as well as implementing said interface.

[notti]

Well layer documentation says "Layer represents a single decoded packet layer (using either the OSI or TCP/IP definition of a layer)." so actually LLDP should have been just one layer. But bear in mind, that we can't change the existing API - so adding DecodeFromBytes to both might be a bit complicated...

[Galadros]

"But bear in mind, that we can't change the existing API" Complicated how so? It's entirely possible that I've misunderstood, but I definitely know how to add DecodeFromBytes to both (it involves splitting up the existing decodeLinkLayerDiscovery method of the LinkLayerDiscovery struct into two DecodeFromBytes() methods and changing the NextLayer() of LinkLayerDiscovery to be LinkLayerDiscoveryInfo, which as far as I'm aware seems to preserve the API?) In effect, the only possible changes to the existing API that I could think of would be adding a canDecode() method to LinkLayerDiscovery (which AFAIK should be fine since nothing should be calling that nonexistent method right now) and that LinkLayerDiscovery.NextLayer() would now return LinkLayerDiscoveryInfo instead of nil. decodeLinkLayerDiscovery would now read:

func decodeLinkLayerDiscovery(data []byte, p gopacket.PacketBuilder) error {
    d := &LinkLayerDiscovery{}
    return decodingLayerDecoder(d, data, p)
}

(I'm using the layer structure in dot11.go as a reference here). Having added DecodeFromBytes() methods to both LinkLayerDiscovery and LinkLayerDiscoveryInfo, decodeLinkLayerDiscovery() should function in an identical fashion- it would add the same layers to the packet in question with the same values. The difference would be that you could now treat either of these layers as a DecodingLayer.

Short version: shift parsing done in decodeLinkLayerDiscovery() to LinkLayerDiscovery.DecodeFromBytes(), then split LinkLayerDiscovery.DecodeFromBytes() in half, with the latter half moving to LinkLayerDiscoveryInfo.DecodeFromBytes().