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

chore(decoders.sflow): Cleanup constant definitions and use switch statements #328

Merged
merged 2 commits into from
Jul 18, 2024

Conversation

srebhan
Copy link
Contributor

@srebhan srebhan commented May 23, 2024

The current FORMAT_* constant definitions in the sFlow decoder mixes definitions for opaque sample_data and flow_data as defined in the sFlow datagram spec and sFlow datastructs spec.
Those definitions reference different things and the current code reuses the definitions in a very confusing way, making it difficult to relate the specification(s) to the code.

The present PR cleans the constant definitions and splits them into their respective meaning. The old FORMAT_* constants are not used anymore internally and are only kept for external users' compatibility.
Additionally, the PR switches to switch statements in the decoding functions as those are more readable and simplifies code a bit.

The PR does NOT apply any functional changes and is for cleanup only.

@lspgn
Copy link
Member

lspgn commented May 24, 2024

Thank you! will have a look.
I wouldn't be too worried about removing instead of deprecating (I may forget to do it next large version bump).

@srebhan
Copy link
Contributor Author

srebhan commented May 24, 2024

@lspgn it's your say, I can also remove the deprecated consts... :-) Shall I?

I do have two more PRs in the pipe, one adding parsing the ETH flow record and one adding discard support, but need to wait on this one as I base them off this PR...

@lspgn
Copy link
Member

lspgn commented May 28, 2024

Yes please go ahead with the removal! Thank you :)

For the other, would you be able to add tests and provide me with a packet capture?

@srebhan
Copy link
Contributor Author

srebhan commented May 29, 2024

Deprecated constants removed. After this PR is merged, I will put up the other two, including unit-tests of course. :-)

@srebhan
Copy link
Contributor Author

srebhan commented Jun 3, 2024

@lspgn is there anything I can do to get this merged?

@lspgn
Copy link
Member

lspgn commented Jun 6, 2024

@srebhan I just need to find some time to take care of it :), thank you

@srebhan
Copy link
Contributor Author

srebhan commented Jul 17, 2024

@lspgn any news?

I also pushed the two dependent feature PRs (#336 and #337) based on this PR. Sorry for pushing but we need the two features to fix our downstream issue... It would be nice if you could find some time to review the PR so I can rebase the mentioned feature PRs. Thanks in advance!

@lspgn lspgn merged commit a0f3df6 into netsampler:main Jul 18, 2024
1 check passed
@lspgn
Copy link
Member

lspgn commented Jul 18, 2024

I'm sorry it took so long to review.
I just merged the three PRs. Thank you for the great work.

lspgn pushed a commit that referenced this pull request Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants