-
Notifications
You must be signed in to change notification settings - Fork 309
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
Implement crun features #1237
Implement crun features #1237
Conversation
2ed1f0b
to
33cb00b
Compare
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.
thanks for working on this. I've left some comments, also make sure make clang-format
passes
208485e
to
dd0ff1b
Compare
62e582c
to
a4605dd
Compare
b6a43b0
to
7c8a4b1
Compare
7c8a4b1
to
4a965a2
Compare
4162ded
to
877ef1c
Compare
877ef1c
to
e2ff6cc
Compare
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.
some more small comments. Also this is a completely new feature, so we need some tests too under tests
to run with make check
eee7303
to
337652b
Compare
src/libcrun/container.c
Outdated
"SCMP_CMP_MASKED_EQ", | ||
}; | ||
|
||
static const char *archs[] = { |
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 am still not sure if we must list the arches. Not only we risk to not list all the supported values, but we also risk that the dynamically linked version of libseccomp doesn't have support for what we are listing here.
What do you think? Wouldn't it be safer for now to just drop the arches list from the JSON output?
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.
Yup, it makes sense. Does that sound good to you if we keep the reference of it in the code for the future but drop the entry from the final output for now?
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 is better to remove the code altogether, we can easily add it back if we ever need it in this form
337652b
to
af8b147
Compare
when I run the new test locally, I get the following error:
|
we can't hardcode the list of capabilities in the test. We only need to verify some well known capabilities are in the list, e.g. |
I can do this
I think you will need to enable seccomp locally before running that test. |
we cannot do that. The kernel could have capabilities that libseccomp doesn't know about, so they could still be missing in the |
af8b147
to
682e103
Compare
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.
LGTM
made some small changes, I'll merge as soon as the CI is green. @sohankunkerkar something that is not clear to me yet, is how will you use this feature to decide whether wasm is supported or not? Wouldn't we need to add that information? |
👍 as a custom annotation (until its JSON property is officially defined in the spec) |
5991ad8
to
1d6c429
Compare
noticed there is something more to fix, I'll push another version |
1d6c429
to
7ab96c6
Compare
I've added a patch. Now wasm support is recorded in a custom annotation |
@flouthoc could you please review the last patch? |
Fixes: containers#1177 Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>
7ab96c6
to
ee3ff35
Compare
@giuseppe patch for |
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.
LGTM
e29dd9d
to
deeef64
Compare
Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
deeef64
to
d006733
Compare
containers/crun#1237 was merged to master and was tagged `crun-1.8.6' on Jun 27, 2023. Signed-off-by: match man <ppucoder@gmail.com>
containers/crun#1237 was merged to master and was tagged `crun-1.8.6' on Jun 27, 2023. Signed-off-by: match man <ppucoder@gmail.com>
1177 fix containers/crun#1237 was merged to master and was tagged `crun-1.8.6' on Jun 27, 2023. Signed-off-by: match man <ppucoder@gmail.com>
1177 fix containers/crun#1237 was merged to master and was tagged `crun-1.8.6' on Jun 27, 2023. Signed-off-by: match man <ppucoder@gmail.com>
The work to support features introduced another point of dependency on libcap that previously wasn't guarded by ifdefs Refs: containers#1237 Signed-off-by: Robert Günzler <r@gnzler.io>
Fixes: #1177