BUG: API level fixups #98
Conversation
|
FYI, I'm pretty sure that 8f38116 is the last commit that I'll push to this branch. I can't spot any other issues. |
tests/39-basic-api_level.c
Outdated
| @@ -28,6 +28,7 @@ int main(int argc, char *argv[]) | |||
| { | |||
| int rc; | |||
| unsigned int api; | |||
| const unsigned int max_api = 2; | |||
There was a problem hiding this comment.
Let's bump the max_api value up to something absurdly high, 1024 for example, and only test to make sure it is flagged as invalid. Otherwise I fear this test is going to be a bit too fragile.
tests/39-basic-api_level.c
Outdated
| @@ -40,5 +41,20 @@ int main(int argc, char *argv[]) | |||
| if (api != 1) | |||
| return -3; | |||
|
|
|||
| rc = seccomp_api_set(max_api); | |||
There was a problem hiding this comment.
See the comment above, let's drop this check.
As odd as it may sound, I'm okay with putting in explicit checks for each supported API level as the API level is added to the library.
There was a problem hiding this comment.
I should have left a comment in the code with my intentions but I was thinking that this would force anyone adding a new API level to add a set/get test for it.
There was a problem hiding this comment.
Yes, I understand, and generally applaud tests like that, but I'm thinking that any new API level addition is going to require additional tests anyway so it isn't as large a concern. I guess I could be convinced that this is the Right Thing To Do if you feel strongly about it.
There was a problem hiding this comment.
I don't feel strongly about it. I just wanted to make sure you knew what I was thinking when I wrote the tests since I did a poor job of recording what I was thinking. I'll get this PR updated hopefully by EOD. Thanks!
tests/39-basic-api_level.py
Outdated
| raise RuntimeError("Failed getting API level 2") | ||
|
|
||
| try: | ||
| set_api(3) |
There was a problem hiding this comment.
Similar to my comments in the C variant of this test, let's bump this up to something really large, e.g. 1024.
|
My experiment at marking individual commits as "approved" failed miserably, so here are the patches I merged:
The other two commits look pretty good, I just had a few small comments (made inline, if things got mangled let me know and I'll just repeat the comments here). |
|
Oh, I forgot to add my thanks for cleaning up my mess! :) |
Test setting all of the valid API levels and then test an invalid API level to ensure that seccomp_api_set() fails. Signed-off-by: Tyler Hicks <[email protected]>
Allow Python applications to get and set the API level using global functions. Signed-off-by: Tyler Hicks <[email protected]>
8f38116
to
a54a57c
Compare
|
I've updated the C and Python test as you requested. I did away with the "max_api" variable in the C test for two reasons:
As a fellow patch-and-mailing-list dinosaur, I feel your pain. :) (though, I'm warming up to this new world)
Pfft. Thanks for helping to clean up all of my messes spread across both PRs. :) |
Here are some minor fixups that I identified while reviewing the API level commit (e89d182).