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

Fix division by zero #19

Closed
wants to merge 1 commit into from
Closed

Fix division by zero #19

wants to merge 1 commit into from

Conversation

fm94
Copy link
Contributor

@fm94 fm94 commented Nov 7, 2022

This might solve issue #18

Background:
The flow length can be in the range of nano/micro-seconds, if it is converted to milliseconds then the uint operation floors it into a 0 which means computing a rate (pkts/time) yields an error. To solve this, the minimum allowed duration for a flow is 1 (unit).

The typical behavior is to return a +inf but since this doesn't make sense in networking I believe that dividing by 1 is better.

@notti
Copy link
Collaborator

notti commented Nov 7, 2022

Please don't modify generated files - but the generator (also look at the file header :) )
Also would look more sensible to me if we either set no value or nil in this case

@fm94
Copy link
Contributor Author

fm94 commented Nov 7, 2022

So maybe we remove divide from gen_math.go because it requires special handling?

I've tested returning Inf or nil but that means dropping a lot of flows just because the flow duration, for instance, is bellow 1 millisec. I'm not sure how to handle this, maybe we should avoid casting into int.

@notti
Copy link
Collaborator

notti commented Nov 7, 2022

That is the wrong place then -> the place where you did the modification is just general division - so you wouldmodifiy division for everybody and everyone to do something to fake a 1 for 0 (for every integer case).
The time conversion happened already at the point where you tried to modified that.
Would be better to fix the number to what you want before - e.g. with an additional computation.

@fm94
Copy link
Contributor Author

fm94 commented Nov 8, 2022

Closing this pull request and creating a new one with a different solution.

@fm94 fm94 closed this Nov 8, 2022
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