-
Notifications
You must be signed in to change notification settings - Fork 5
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 program crash on "m" with large value of "current" #10
base: main
Are you sure you want to change the base?
Conversation
Fix program crash if the "m" command is used and "current" is higher than supported by the current stats array length.
Reset stats_history[] when switching the transform function - AFAICT the contents of the stats_history array depends on the transformation performed on the gathered statistics data.
Improve response time of "m" command to switch between device centric and measurement centric views
I don't have a bug report so I don't know what it's fixing yet. The stats array can change? As in, the device inventory can change, or the dimensions of measurement can change? |
The value of "current" is limited to (0 .. length(stats)-1) at the end of the loop. But when "m" is used to switch the mode, "current" may be too large for the array. The pull request consists of 3 parts:
I had missed the fact that current must be within bounds of an array that changes size depending on the device vs. measurements display mode when submitting my previous pull request. These changes make the use of "m" safe and immediately resulting in the selected display mode. |
Ah.
|
@@ -225,14 +225,19 @@ def render_stats(window, transform, should_show_differential, pool, filename=Non | |||
elif in_key == ord('d'): | |||
should_show_differential = not should_show_differential | |||
elif in_key == ord('m'): | |||
stats = None | |||
stats_history = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of discarding recent history, I'd prefer it if stats and stats_history were unaffected by views and transforming functions were only used to interpret the raw data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do not want to clear the stats_history array, I'd assume that you need two arrays, one for the device centric view, the other for the measurements centric view.
Always applying both transforms and just selecting one of 2 stats and stats_history arrays for display would be a simple change, but my main intend was to provide a fix for the issue that resulted from the addition of the "m" command.
If you provide 2 sets of arrays and put the restriction of "current" to the allowed index range at the start of the loop, then resetting of current to 0 could also be dropped.
I'll update the FreeBSD port to whatever you decide to implement.
Fix program crash if the "m" command is used and "current" is higher than supported by the current stats array length.
The "m" command has been part of a previous pull request that has been merged. I forgot that the stats array length can change and that the stats_history contents depends on the transform function selected.
This patch fixes this issue. The value of "current" is reset to one that is valid for all possible stats array lengths, and stats_history is reset to an empty array to prevent incompatible data to be used in a difference calculation.