Extracted MTU check code into its own helper functions.

[Dimi1010]

This PR separates the logic for checking packets against the device MTU size by moving validation code to checkMtu helper functions.

It also renames sendPacketDirect to sendPacketUnchecked to improve readability.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 69.86301% with 22 lines in your changes missing coverage. Please review.

Project coverage is 82.96%. Comparing base (26b8485) to head (23b7df1). Report is 1 commits behind head on dev.

Files with missing lines	Patch %	Lines
Pcap++/src/PcapLiveDevice.cpp	68.11%	17 Missing and 5 partials :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1834      +/-   ##
==========================================
- Coverage   83.00%   82.96%   -0.04%     
==========================================
  Files         285      285              
  Lines       49013    49062      +49     
  Branches    10313    10376      +63     
==========================================
+ Hits        40681    40703      +22     
- Misses       7180     7208      +28     
+ Partials     1152     1151       -1     

Flag	Coverage Δ
alpine320	75.04% <60.37%> (-0.05%)	:arrow_down:
fedora42	75.15% <60.37%> (-0.04%)	:arrow_down:
macos-13	80.46% <60.93%> (-0.06%)	:arrow_down:
macos-14	80.47% <60.93%> (-0.05%)	:arrow_down:
macos-15	80.45% <60.93%> (-0.05%)	:arrow_down:
mingw32	71.27% <56.60%> (-0.09%)	:arrow_down:
mingw64	71.25% <56.60%> (-0.11%)	:arrow_down:
npcap	84.90% <66.10%> (-0.10%)	:arrow_down:
rhel94	74.91% <60.37%> (-0.07%)	:arrow_down:
ubuntu2004	58.64% <59.64%> (+<0.01%)	:arrow_up:
ubuntu2004-zstd	58.76% <59.64%> (-0.02%)	:arrow_down:
ubuntu2204	74.86% <60.37%> (-0.04%)	:arrow_down:
ubuntu2204-icpx	61.42% <53.12%> (-0.07%)	:arrow_down:
ubuntu2404	75.08% <60.37%> (-0.07%)	:arrow_down:
ubuntu2404-arm64	75.06% <60.37%> (-0.05%)	:arrow_down:
unittest	82.96% <69.86%> (-0.04%)	:arrow_down:
windows-2019	84.92% <66.10%> (-0.10%)	:arrow_down:
windows-2022	84.94% <66.10%> (-0.09%)	:arrow_down:
winpcap	85.09% <66.10%> (-0.08%)	:arrow_down:
xdp	50.50% <0.00%> (-0.06%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Dimi1010]

• It doesn't prevent code duplication because each checkMtu() override is only used once (unless I'm missing something)

The initial idea was to separate the logic for checking a packet against the device's MTU into its own function.

The new check functions can also be added as part of the public API of a device as pure checks. I didn't make them public initially to keep it simple for now.

IMO, doMtuCheck doesn't work that good as a pure check function in the API due to it raising an error message when the check fails. The user may want a silent check and do some other action on failure silently.

• IMO, it doesn't really make the code easier to read

It somewhat simplifies all the sendPacket flow into:

1. Conditionally call checkMtu and log error on fail.
2. Call sendPacketUnchecked.

IMO, that is easier to read and understand, instead of having to have sendPacket overloads to call each other in circles.

• Some of the checkMtu() overloads have a bit strange API, like the one that checks the MTU but also returns the packet length

Yeah, the out param is a bit strange, but it is the best way to optionally get the packet length that the packet was parsed to without needing to reparse the packet. That can be useful info if the check fails and the user wants to log the error. It was also the best way to keep the return value consistent across overloads.

Hope that answers your questions. :)

[seladb]

Having both doMtuCheck() as a public method and checkMtu() as a protected method is even more confusing IMO. I'm ok with removing the error log in doMtuCheck() if you think it's a cleaner API

[Dimi1010]

IMO, having the checkMtu as public and doMtuCheck possibly deprecated/removed would be better? None of the other public methods have the naming structure of doMtuCheck, so it kinda sticks out to me. The other do*** methods are mostly protected implementation methods like doOpen.

[Dimi1010]

Although on second thought, perhaps checkMtu is a bit ambiguous as it might be interpreted as a check for the MTU instead of against the MTU.

Perhaps changing the method name to fitsInMtu or something similar would be better?

[Dimi1010]

@seladb Changed the function name to be more explicit and moved the new functions to public API. Also added a new section to the PR post explaining how the change improves readability by simplifying the control flow of the device internals.

[seladb]

@Dimi1010 I'm still not convinced this change is needed. We add multiple public methods to PcapLiveDevice which makes the API more complex for no apparent reason. Also - the existing logic (before this change) is clear and readable enough and I don't think we need this refactoring

[Dimi1010]

@seladb IDK, about the previous internal logic being clear. I spent a significant amount of time trying to wrap my head around the original call chains of sendPacket initially, due to them going in both directions depending on the checkMtu flag.

When checkMtu is false, the overloads defer in the direction of Packet -> RawPacket -> C array. When checkMtu is true, the overloads defer in the direction of C array -> RawPacket -> Packet -> RawPacket -> C array.

IMO, the MTU check's logic is complex enough that having it as a dedicated function family improves the readability as it abstracts away the "backwards" control flow in sendPacket from C array to Packet due to needing to parse the packet.

It provides two distinct logical units to think about:

• sendPacket - defers between overloads always go Packet -> RawPacket -> C array
• isPayloadWithinMtu - defers between overloads always go C array -> RawPacket -> Packet

Instead the previous implementation had a single logic unit that conditionally branches on every possible level like this:

• C array (A) -> Can sometimes go to (B) and sometimes transmits.
• RawPacket (B) -> Can sometimes go to (A) and sometimes go to (C)
• Packet (C) -> Can sometimes go to (B) and sometimes do MTU check and go to (A)

We add multiple public methods to PcapLiveDevice which makes the API more complex for no apparent reason.

I have no strong opinion on if the helper methods that encapsulate the MTU validation logic are public or not. It is mostly because doMtuCheck is already public and we do provide the ability for users to perform a query against the MTU size, that the new functions are public.

In this respect, the extra overloads can be considered utility functions of convenience, so the query can be performed against more common library objects (i.e., Packet, RawPacket), instead of having the user manually have to extract the payload length.

[seladb]

ok @Dimi1010 let's go with your suggestion. But can we make the new MTU check methods private? We can leave the existing doMtuCheck() public even though it's not ideal. Please let me know what you think

[Dimi1010]

@seladb moved the declarations to protected, or do you want them explicitly in private?