Skip to content
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

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

Open
wants to merge 22 commits into
base: dev
Choose a base branch
from
Open
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
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
Copy link
Owner

Choose a reason for hiding this comment

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

Why update this file?

Copy link
Author

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
@@ -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;

@@ -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...);
}

@@ -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;
3 changes: 2 additions & 1 deletion Common++/header/Logger.h
Original file line number Diff line number Diff line change
@@ -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
6 changes: 6 additions & 0 deletions Packet++/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -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
@@ -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
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