Skip to content

fix(attach-cgroup): only attach to a cgroup if it is already set up#52

Merged
kaniini merged 1 commit intomainfrom
fix/attach-cgroup
Dec 19, 2025
Merged

fix(attach-cgroup): only attach to a cgroup if it is already set up#52
kaniini merged 1 commit intomainfrom
fix/attach-cgroup

Conversation

@kaniini
Copy link
Copy Markdown
Contributor

@kaniini kaniini commented Dec 19, 2025

No description provided.

Comment thread src/wrap.rs
let path_str = path
.to_str()
.ok_or(anyhow!("path is somehow not valid utf-8"))?;
let subtree = CGroup::open(path_str)?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add .context() here?

Comment thread src/wrap.rs
.unwrap_or("/sys/fs/cgroup".to_string());
let cgroot = CGroup::open(&cgbase)?;
let subtree = cgroot.create_child(format!("styrolite-{}", self.identity()?))?;
let name = format!("styrolite-{}", self.identity()?);
Copy link
Copy Markdown
Contributor

@bleggett bleggett Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to create this at all?

Why not just try and open whatever cgroup path is given by the caller, regardless of what it is, and attach to it, regardless of whether it has this path segment in it?

Another option is to check if we got the default path of /sys/fs/cgroup and only append the custom path specifier in that case, otherwise take the path that's given and attach.

Comment thread src/wrap.rs
path.push(&name);

if !path.exists() {
return Ok(());
Copy link
Copy Markdown
Contributor

@bleggett bleggett Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to return with no error and no binding if the path doesn't exist? Seems like we should warn here, since we effectively are skipping attach without a warn/error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be an API change. I don't want to change the API right now.

@kaniini kaniini merged commit a33e2e1 into main Dec 19, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants