-
Notifications
You must be signed in to change notification settings - Fork 187
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
CFE-3020: ps output line ... is shorter than its associated header #5082
base: master
Are you sure you want to change the base?
Conversation
A program can overwrite its incoming arguments. This can include blanking them out, and subsequent "ps" output can be empty. Add an additional test. While this test itself does not expose or address the CFE-3020 bug, nevertheless it seems worth having such a test in place.
Thanks for submitting a pull request! Maybe @olehermanse can review this? |
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.
Thank you for working on this. I've left some questions and suggestions on individual lines of code. Additionally, this is not passing tests as there is some illegal memory access going on in one of the assert_string_equal
lines. Feel free to comment out parts of the test to see which line is problematic. I assume you can see the test results and failures in GitHub?
|
||
assert_true(SplitProcLine(lines[1], pstime, name, start, end, PCA_AllColumnsPresent, field)); | ||
assert_string_equal(field[user], "spacecmd"); | ||
assert_string_equal(field[command], lines[1] + 69); |
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.
I think it's easier and less error prone if you just compare to what string you expect here, for example:
assert_string_equal(field[command], lines[1] + 69); | |
assert_string_equal(field[command], ""); |
assert_true(SplitProcLine(lines[2], pstime, name, start, end, PCA_AllColumnsPresent, field)); | ||
assert_string_equal(field[user], "nullcmd"); | ||
assert_string_equal(field[command], lines[1] + 69); |
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.
Same as above, also unclear whether the lines[1]
here is intentional or not? We are working on lines[2]
now.
assert_true(SplitProcLine(lines[2], pstime, name, start, end, PCA_AllColumnsPresent, field)); | |
assert_string_equal(field[user], "nullcmd"); | |
assert_string_equal(field[command], lines[1] + 69); | |
assert_true(SplitProcLine(lines[2], pstime, name, start, end, PCA_AllColumnsPresent, field)); | |
assert_string_equal(field[user], "nullcmd"); | |
assert_string_equal(field[command], ""); |
"spacecmd 4 S 0 0 0 1 10:30 54:29 00:00:01 ", | ||
"nullcmd 4 S 0 0 0 1 10:30 54:29 00:00:01 " |
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.
Why does line 1 end with 2 spaces and line 2 end with 1 space?
/* Indent any continuation lines so that it's clear that's what they are. */ | ||
/* Use the username field as a test name to confirm tests are in sync. */ | ||
|
||
"USER PID STAT VSZ NI RSS NLWP STIME ELAPSED TIME COMMAND", |
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 this is exactly copied from a (ps) command, putting the command in a comment here would be helpful.
If not, is it intentional / important that the labels and values are sometimes misaligned? It could help readability if you added some spaces to align things perfectly.
"spacecmd 4 S 0 0 0 1 10:30 54:29 00:00:01 ", | ||
"nullcmd 4 S 0 0 0 1 10:30 54:29 00:00:01 " | ||
}; | ||
char *name[CF_PROCCOLS] = { 0 }; /* Headers */ |
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.
headers
or labels
would be a better name here.
A program can overwrite its incoming arguments. This can include blanking them out, and subsequent "ps" output can be empty.
Add an additional test.
While this test itself does not expose or address the CFE-3020 bug, nevertheless it seems worth having such a test in place.