-
Notifications
You must be signed in to change notification settings - Fork 140
include the correct header for syscall definitions #558
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
base: main
Are you sure you want to change the base?
Conversation
This file needs to utilize `__NR_seccomp`, which is defined in the linux uapi headers, not seccomp.h, even though seccomp.h does itself indirectly cause this header to be included as well. Nothing else in this particular file needs seccomp.h so drop that include altogether since it's now entirely unused. Signed-off-by: Eli Schwartz <[email protected]>
|
Assuming more appropriate is to |
| #include <sys/sysmacros.h> | ||
| #include <linux/seccomp.h> | ||
| #include <seccomp.h> | ||
| #include <asm/unistd.h> |
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.
Assuming more appropriate is to #include <syscall.h> or #include <sys/syscall.h> as seccomp_notify.c needs this to call the __NR_seccomp syscall?
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.
So, this does not fix anything, except maybe simplifies things a little.
There must be an underlying reason for this change, can you please share it @eli-schwartz ?
|
The motivation here is to be able to drop the |
|
Hi, sorry for the delay in responding (thanks Sam for prodding me). As Sam said, the underlying goal here was trying to remove unneeded deps. I was trying to figure out where libseccomp was(n't) used and this seemed to jump out at me early on. I've looked a bit deeper, and it seems this could in theory be useful for EOL kernels, but shouldn't try to support unsupported EOL kernels. However if we do want that it should be a configure time kernel check... I can update to do that instead if you prefer. WDYT about requiring kernel 5.0? |
sorry for the delay to get to this. That is fine. |
|
@eli-schwartz Are you planning to update your PR based on your comment?
|
This file needs to utilize
__NR_seccomp, which is defined in the linux uapi headers, not seccomp.h, even though seccomp.h does itself indirectly cause this header to be included as well.Nothing else in this particular file needs seccomp.h so drop that include altogether since it's now entirely unused.