-
Notifications
You must be signed in to change notification settings - Fork 70
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
Print error message on empty BPF #192
base: master
Are you sure you want to change the base?
Conversation
Thanks for the contribution! For unknown reasons the test seems hang on CI, will see what happened when the job aborts. |
It looks really broken :P |
Weird, it seems caused by this PR: https://github.com/david942j/seccomp-tools/runs/6949456039?check_suite_focus=true
The last message before hang was the log @qua3k did you try running tests with your change locally? You need to do something like
|
I've found the issue; at dump_spec.rb#L37 there is a test with a limit of 2, but the program only has one filter installed. This worked fine previously, but the change breaks it and it fails. I'm not sure how you want to proceed with this -- do you want to modify the test? ++ additionally, we could separate ENOENT and EINVAL to print unique messages (no filter at this index/no filters installed for process) here's the error:
|
Thanks for the investigation!
Yes I prefer to differentiate the two cases. Like the test case shows if there is only one filter installed but set limit to 2, the output would look weird. So let's handle the case properly. |
This is useful when we're not sure if a filter is installed or not
I fixed the test failures but am unsure how to stop it from printing the "no filter exists at this index" on test runs |
Logger.error('No seccomp filters installed') | ||
break | ||
rescue Errno::ENOENT | ||
Logger.error('No filter exists at this index') |
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.
For the test case with limit = 2, it's fine to print this message.
But for the case of limit is negative (which means no limit), we shouldn't print that message as it's expected to just stop when there is no filters, it's not an error. So let's add an if limit.positive?
condition at this line.
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.
Hi @qua3k , do you want to make this change?
Apologies in advance if this is unwanted/breaks API, but I just found it nice to know when no seccomp filters were installed. I am not a Ruby dev by profession so this may be wrong (but it works on my machine)