-
Notifications
You must be signed in to change notification settings - Fork 999
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
rustjail: fix the issue of invalid cgroup_parent path #694
Conversation
/test |
@@ -1404,9 +1406,9 @@ impl Manager { | |||
} | |||
|
|||
let p = if value == "/" { | |||
format!("{}{}", mnt.unwrap(), cpath) | |||
format!("{}{}", mnt.unwrap(), cgroup_path) |
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.
Could you also change these unwrap()
's to map_err()?
's?
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 @jodh-intel
Those unwraps are on Option variables and they had been tested with is_some(), so here it's safe to unwrap on them.
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.
what happen if cpath
doesn't start with '/' ?, cgroup_path
will be empty, so p
will be equal to mnt.unwrap()
, is this expected ?
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 @devimc good point, I had updated.
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.
What about just using format!("{}/{}", xxx, xxx)
, adding an extra /
in between in all cases?
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.
Yeah, that's an option, which let the kernel do the '/' deduplication.
/test |
The cgroup_parent path is expected to be absolute path, add an '/' prefix to the passed cgroup_parent path to make sure it's an absolute path. Fixes: kata-containers#336 Signed-off-by: fupan.lfp <fupan.lfp@antfin.com>
/test |
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 - thanks @lifupan
lgtm, I can confirm it fixed the issue for me #336 |
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!
The cgroup_parent path is expected to be absolute path,
if it was passed a relative path, change it to be an
absolute path.
Fixes: #336
Signed-off-by: fupan.lfp fupan.lfp@antfin.com