ID 38 is already taken by DHCPv6 protocol. The next available ID is 58, please add it after GTPv2

Why removing this cppcheck-suppress comment?

Cppcheck flags this as an incorrect suppression on my local machine. Let me know if you think I should revert it.

That is interesting. Which version of CPPcheck are you using?

2.7 as recommanded in CONTRIBUTING.md and still flags incorrect suppression, I'm keeping this changes so can I commit my recents updates,

Why update this file?

same thing as my previous comment, I only removed some suppress-checks detected as incorrect

Can you add one pcap file with all of these packets?

yes sure, I'll revert suppress-checks and add pcap file containig all tested packets in next PR, also try to cover some missing checks based on codecov feedback

Is this for another PR? If so, should the remaining data be parsed as a generic payload layer for now?

m_NextLayer is intended to be the UDS layer, which has not been implemented yet. In the future, as more knowledge is gained, either I or another contributor may add this functionality. For now, I suggest parsing it as a generic layer, as you mentioned.
PS: the nextLayer will be parsed only when the payloadType is 0x8001.

your feedback is highly appertiated, what do you think about adding this snippet of code:

void  parseNextLayer() override
		{
			if (getPayloadType() == DoIpPayloadTypes::DIAGNOSTIC_MESSAGE_TYPE)
			{
				size_t headerLen = sizeof(doiphdr);

				if (m_DataLen <= headerLen + 2 /*source address size*/ + 2 /*target address size*/)
					return;

				uint8_t* payload = m_Data + (headerLen + 2 + 2);
				size_t payloadLen = m_DataLen - (headerLen + 2 + 2);
				m_NextLayer = new PayloadLayer(payload, payloadLen, this, m_Packet);
				return;
			}
		}`

your feedback is highly appertiated, what do you think about adding this snippet of code:

void  parseNextLayer() override
		{
			if (getPayloadType() == DoIpPayloadTypes::DIAGNOSTIC_MESSAGE_TYPE)
			{
				size_t headerLen = sizeof(doiphdr);

				if (m_DataLen <= headerLen + 2 /*source address size*/ + 2 /*target address size*/)
					return;

				uint8_t* payload = m_Data + (headerLen + 2 + 2);
				size_t payloadLen = m_DataLen - (headerLen + 2 + 2);
				m_NextLayer = new PayloadLayer(payload, payloadLen, this, m_Packet);
				return;
			}
		}`

Seems good. Can the source address and target address used in that type of message be accessed from this layer? Since we are excluding it from the generic payload.

A minor tip, having headerLen be marked as constexpr might allow some compiler optimizations (such as the arithmetic using it + a literal being computed during compilation and hardcoded if possible).

Also why the return statement that is at the end of the block anyway?

@Dimi1010 , this approach appears both safer and cleaner. It allows us to construct a DiagnosticMessage directly from the current layer, providing direct access to the diagnostic data. Using this data, we can then build a generic PayloadLayer.

void parseNextLayer() override
{
	DiagnosticMessageData diagnosticMessage;

	if (diagnosticMessage.buildFromLayer(this))
	{
		m_NextLayer = new PayloadLayer(diagnosticMessage.diagnosticData.data(),
				                             diagnosticMessage.diagnosticData.size(), this, m_Packet);
	}
}

buildFromLayer safely parses the current layer and verifies whether it represents a valid diagnostic message.
what do you think ?

Sure, but my question stands. DiagnosticMessageData has two other members (sourceAddress and targetAddress) which at the moment I don't see how the user can access them easily. They have neither accessors in the current DoIPLayer or are included as part of the UDSLayer (currently PayloadLayer).

sourceAddress and targetAddress are public members of DiagnosticMessageData class just like diagnosticData and they are not part of the UDS layer:

uint16_t sourceAddress;              /**< Source address of the message. */
uint16_t targetAddress;              /**< Target address for the diagnostic message. */
std::vector<uint8_t> diagnosticData; /**< Diagnostic message data with dynamic length. */

the user can access these fields by the method buildFromLayer .
I've made this dummy function just to show how to access to these fields :

void DoIpLayer::resolveDiagMessageFields()
	{
		DiagnosticMessageData diagnosticMessage;
		if (diagnosticMessage.buildFromLayer(this))
		{
			uint16_t srcAddr = diagnosticMessage.sourceAddress;
			uint16_t targetAddr = diagnosticMessage.targetAddress;
			std::vector<uint8_t> diagData = diagnosticMessage.diagnosticData;
		}
	}

Do you think this implementation need more improvement ?

Ok, if i am understanding correctly the sequence is this?

1. User somehow receives a DoIPLayer from a Packet
2. User checks the payload type (via DoIPLayer::getPayloadType())
3. Depending on the payload type user uses T::buildFromLayer(DoIPLayer) (T being the corresponding message struct) to populate the data from the layer into the struct.

Am I understanding the flow correctly?

yes you are absolutely correct.

Sure, that works fine. 👍

Sure, but my question stands. DiagnosticMessageData has two other members (sourceAddress and targetAddress) which at the moment I don't see how the user can access them easily. They have neither accessors in the current DoIPLayer or are included as part of the UDSLayer (currently PayloadLayer).

IMO, it would be better to have the enum to string maps be converted to functions with switch statements. It would allow to expose a cleaner and uniform public API (toString(EnumType value) or toDescriptionString(EnumType value)) for the different enums. I also ran a benchmark and it does end up slightly faster (at least for DoIpProtocolVersion) due to some compilers producing a jump table (GCC and Clang do, MSVC doesn't).

The functions can then be folded into DoIpEnums.h with the implementations into DoIpEnums.cpp.

Thanks for your feedback, @Dimi1010

In my humble opinion, unordered_map looks much cleaner and more declarative compared to switch statements, which can sometimes feel cluttered with redundant return or break statements. I’ve also ensured that edge cases, like missing keys, are handled appropriately whenever I use these maps.

To be honest, I haven’t focused much on the potential performance impact, as I believe the effect with these small enums would be negligible, though I understand it might require additional effort.

That said, if the difference in performance isn’t significant, I think we can keep it as it is. However, if you strongly feel this change is necessary, I’ll plan to prepare a fix over the weekend.

Thank you for your understanding!

Sure. Not necessarily a strong opinion on the switches. They can stay declared as maps.

Something else that occurred to me. The maps are declared as static in this header. For a global variable to be declared static that mean that is has internal linkage in the translation unit. For a header that means that each translation unit that header is included in, gets its own enum to string map. This is an issue as it leads duplicated maps in the different translation units. Please remove the static keyword from the maps.

Additionally, Are the maps supposed to be part of the public API or to be used only through the methods in DoIpLayerData?

• If they aren't supposed to be part of the public API, please put them in the pcpp::internal namespace.
• If they are to be part of the public API, I think that maybe having a function encapsulate the search/conversion would be better, even if the conversion is done with maps, as it detaches the implementation details of the conversion from the public API. What do you think?

@Dimi1010 , thank you for your feedback!
Once again, your remarks are clear and insightful. The EnumToStringMaps are designed to be included only in DoIpLayerData, which avoids duplicate data in this context. However, for future designs, if anyone tries to use it in another translation unit, it could lead to increased memory consumption. I’ll definitely remove the static keyword as you suggested.
For the second question, yes, these maps are specific to the DoIP context, so they aren’t intended to be part of the public API. I’ll move them to the pcpp::internal namespace for better clarity and data organization.