search

netsampler / goflow2

High performance sFlow/IPFIX/NetFlow Collector
BSD 3-Clause "New" or "Revised" License
487 stars 112 forks source link

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

Closed srebhan closed 4 months ago

srebhan commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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

srebhan commented 5 months ago

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

lspgn commented 5 months ago

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

srebhan commented 4 months ago

@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 commented 4 months ago

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