Skip to content

Add Layer Build and Validation for DoIP (Diagnostic over IP) Support #1655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 57 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
a50cf68
Add Layer Build and Validation for DoIP (Diagnostic over IP) Support
Dec 8, 2024
703b650
Merge branch 'master' into develop
raissi-oussema Dec 7, 2024
7aee737
Merge branch 'dev' into develop
tigercosmos Dec 8, 2024
7966b06
Correct endianness handling for imported data to ensure consistent in…
Dec 9, 2024
d4658d7
Fix serval issues related to multi-cross platform
Dec 9, 2024
dd26c31
other fix for DoIpLayerData
Dec 9, 2024
2ed8c0a
.add doipPackets.pcapng containg all doip packets.
Jan 6, 2025
0d52dbf
Merge branch 'dev' into develop
raissi-oussema Jan 7, 2025
b317bf6
Merge branch 'dev' into develop
raissi-oussema Jan 8, 2025
3ba3c71
fix dioxygene and gersemi errors
Jan 9, 2025
7822458
Merge branch 'dev' into develop
raissi-oussema Jan 9, 2025
3c5dc4d
Merge branch 'dev' into develop
raissi-oussema Jan 12, 2025
7896a9e
Update Packet++/header/DoIpLayer.h
raissi-oussema Jan 15, 2025
c159be9
Update Packet++/src/DoIpLayer.cpp
raissi-oussema Jan 15, 2025
14bc5a5
add diff improvement and dioxygene style
Jan 16, 2025
0545afe
. make safe search in enumsToString, add parseNextLayer for UDS layer…
Jan 18, 2025
14c9842
remove buildFromLayer from IDoIpMessageData
Jan 19, 2025
c4b8baa
fix dioxygene pipeline
Jan 19, 2025
326ae50
change doc from pointer to const ref
Jan 19, 2025
9d97fce
try fix dioxygene
Jan 19, 2025
031b36c
try resolve doxygene
Jan 22, 2025
929287b
move maps to internal namespace
Jan 23, 2025
0738b7e
Merge branch 'dev' into develop
raissi-oussema Jan 29, 2025
5b18d91
fix log errors
Feb 4, 2025
4d8f4c0
Merge branch 'dev' into develop
raissi-oussema Feb 4, 2025
f205483
revert suppress checks
Feb 4, 2025
5725fbb
revert cpp checks
Feb 4, 2025
df43de8
Merge branch 'dev' into develop
seladb Mar 19, 2025
4986248
. Change comment style from /** */ to ///
Mar 21, 2025
879f5b0
Merge branch 'dev' into develop
raissi-oussema Mar 30, 2025
d2204c0
. Reply on 1655#pullrequestreview-2707929155
Mar 30, 2025
1453f8a
fix clang format in ProtocolType.h
Mar 30, 2025
05e147e
Update ProtocolType.h
raissi-oussema Mar 30, 2025
47e2e16
try fix clang format
Mar 31, 2025
c205897
serval fixes
Apr 8, 2025
090ab0e
fix doxygene pipeline
Apr 10, 2025
301b0c3
.Big refactoring:
Apr 19, 2025
a496b74
Merge branch 'dev' into develop
raissi-oussema Apr 19, 2025
f06112e
fix clang indentation error
Apr 19, 2025
0c7a19a
try fix pipeline
Apr 20, 2025
51e5de4
group doipConstants in struct and other improvements
Apr 20, 2025
bb00786
fix doxygene and bool data initialisation
Apr 20, 2025
66002a8
serval improvements
Apr 24, 2025
97ce5d1
try fix doxygene and add base Diagnotic response Class for diagnostic…
Apr 25, 2025
a274d70
try fix doxygen and remove unused imports
Apr 25, 2025
6ddfe8e
fix review request
Apr 30, 2025
d6c116b
fix review comments
May 6, 2025
cc6d70c
Apply code review suggestions and resolve feedback comments
May 11, 2025
c7fd664
fix endian import
May 13, 2025
e1beca1
Reduce reservedIso variable scope
May 13, 2025
ae6e41c
fix review comments
May 19, 2025
ecac27e
serval improvements
May 20, 2025
7bd7276
try fix shorten/extend logic
May 21, 2025
885809a
fix lenght adjustement
May 23, 2025
fd21439
enhance DoIpTests.cpp
Jun 10, 2025
2bfbf65
enhance test spec
Jul 16, 2025
b27ba89
Improve the consistency of naming conventions and the structure of im…
Jul 18, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions 3rdParty/json/include/json.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -3406,7 +3406,7 @@ NLOHMANN_JSON_NAMESPACE_END
template<typename U> class AllocatorType = std::allocator,
template<typename T, typename SFINAE = void> class JSONSerializer =
adl_serializer,
class BinaryType = std::vector<std::uint8_t>, // cppcheck-suppress syntaxError
class BinaryType = std::vector<std::uint8_t>,
class CustomBaseClass = void>
class basic_json;

Expand Down Expand Up @@ -4251,7 +4251,6 @@ inline std::size_t concat_length(const char /*c*/, const Args& ... rest)
template<typename... Args>
inline std::size_t concat_length(const char* cstr, const Args& ... rest)
{
// cppcheck-suppress ignoredReturnValue
return ::strlen(cstr) + concat_length(rest...);
}

Expand Down Expand Up @@ -19991,7 +19990,6 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
#if JSON_DIAGNOSTICS
JSON_TRY
{
// cppcheck-suppress assertWithSideEffect
JSON_ASSERT(!check_parents || !is_structured() || std::all_of(begin(), end(), [this](const basic_json & j)
{
return j.m_parent == this;
Expand Down
3 changes: 2 additions & 1 deletion Common++/header/Logger.h
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ namespace pcpp
PcapLogModuleKniDevice, ///< KniDevice module (Pcap++)
PcapLogModuleXdpDevice, ///< XdpDevice module (Pcap++)
NetworkUtils, ///< NetworkUtils module (Pcap++)
NumOfLogModules
PacketLogModuleDoIpLayer, ///< DoipLayer module (Packet++)
NumOfLogModules,
};

/// Cross-platform and thread-safe version of strerror
Expand Down
6 changes: 6 additions & 0 deletions Packet++/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ add_library(
src/DnsLayer.cpp
src/DnsResource.cpp
src/DnsResourceData.cpp
src/DoIpLayer.cpp
src/DoIpLayerData.cpp
src/EthDot3Layer.cpp
src/EthLayer.cpp
src/FtpLayer.cpp
Expand Down Expand Up @@ -80,6 +82,10 @@ set(
header/DnsLayer.h
header/DnsResourceData.h
header/DnsResource.h
header/DoIpEnums.h
header/DoIpEnumToString.h
header/DoIpLayer.h
header/DoIpLayerData.h
header/EthDot3Layer.h
header/EthLayer.h
header/FtpLayer.h
Expand Down
226 changes: 226 additions & 0 deletions Packet++/header/DoIpEnumToString.h
Copy link
Collaborator

@Dimi1010 Dimi1010 Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator

@Dimi1010 Dimi1010 Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Large diffs are not rendered by default.

Loading
Loading