Skip to content

Conversation

@corubba
Copy link
Contributor

@corubba corubba commented Oct 19, 2025

Limit the access of the slow_print function to the subtype field, which is the only field the slow protocol defines. This means moving the version stuff for LACP and Marker to their own dedicated functions.

The output is kept mostly the same as before, which is why there is no change to the test data. The only deliberate change is replacing the "version not supported" message with printing the packet data verbatim.

This refactor was suggested in #1363. While I understand the reason, after seeing the actual change/result, I am personally not entirely convinced it is actually an improvement. But I will provide it for your consideration and judgement.

Limit the access of the `slow_print` function to the subtype field,
which is the only field the slow protocol defines. This means moving
the version stuff for LACP and Marker to their own dedicated functions.

The output is kept mostly the same as before, which is why there is no
change to the test data. The only deliberate change is replacing the
"version not supported" message with printing the packet data verbatim.
@infrastation
Copy link
Member

Thank you for attempting this code clean-up. It looks better, in that the subtype-specific length validation and version handling no longer obscure the main purpose of slow_print().

Note that the two new functions (slow_lacp_print() and slow_marker_print()) are mostly duplicates, the checks they make could be done just before the while loop in slow_marker_lacp_print(), which receives the subtype as an argument anyway, so in the existing function it would be trivial to enforce the minimum length requirement, to fetch and to validate the 8-bit version and to print the protocol name. Does it make sense?

Consolidate the subtype-functions into the common function.
@corubba
Copy link
Contributor Author

corubba commented Oct 20, 2025

That was my original approach, but I didn't like the control flow. Especially since you need to check the subtype+version tuples, and that becomes quite ugly if you want to future-proof for LACPv2 (which is a thing). So I moved the whole version stuff to their own subtype-functions, which made the control flow a lot cleaner as each protocol just switches on its own versions, and leaves the common function as a pure TLVv1 implementation.

Having tinkered with it a bit more now, it's better ... but I somehow still like the flow of other version better, even if it contains duplicated code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants