Re: [PATCH v2] Set up control groups for most services

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 25260 bytes --]

Demi Marie Obenour <demiobenour@gmail.com> writes:

> On 6/24/26 08:13, Alyssa Ross wrote:
>> Demi Marie Obenour <demiobenour@gmail.com> writes:
>> 
>>> The cgroups are handled by a Rust tool.  The name of the cgroup is
>>> autogenerated from the service name.
>>>
>>> Using cgroups for process control is not implemented yet.
>>>
>> 
>> Perhaps you could include some more details of how the cgroup hierarchy
>> is supposed to be set up and why, either in the patch body or in
>> documentation?
>
> Will fix in v3.
>
>> As I understood it, the point of using cgroups was that we could bundle
>> all services for a VM, including the VMM, into a single cgroup, but I
>> don't see that here, just a pure translation of the service hierarchy
>> (which the VMM might not even be part of).  Are per-VM cgroups coming
>> later?
>
> I think it would be significantly simpler to have the VMM be
> just another VM service.  This would naturally put it under the
> vm-services cgroup, solving this problem.  Since this would mostly
> involve changes to execline scripting, I think it would be better if
> you wrote this code.

No, I don't think it can work that way, because the VMM is in
increasingly many cases not a service at all, since it's run-once.
(Think about e.g. an appimage VM.)  This is why it's not under
vm-services today.  (I thought we'd discussed this before, but it would
have been months ago given how long ago we planned this…)

>>> diff --git a/host/rootfs/image/etc/s6-linux-init/run-image/service/serial-getty/run b/host/rootfs/image/etc/s6-linux-init/run-image/service/serial-getty/run
>>> index 78f794202bf174f3c036f3e20755ac087a988277..ed0808b9e45b077023f237a0f9c0e5c830015ac2 100755
>>> --- a/host/rootfs/image/etc/s6-linux-init/run-image/service/serial-getty/run
>>> +++ b/host/rootfs/image/etc/s6-linux-init/run-image/service/serial-getty/run
>>> @@ -1,5 +1,6 @@
>>> -#!/bin/execlineb -WP
>>> +#!/bin/execlineb -WS1
>>>  # SPDX-License-Identifier: EUPL-1.2+
>>>  # SPDX-FileCopyrightText: 2023 Alyssa Ross <hi@alyssa.is>
>>>  
>>> +cgroup-setup -- $1
>>>  s6-svscan -d3 instance
>> 
>> We're putting supervisors of instances services in a cgroup?  Can you
>> explain to me why that's useful?
>
> This allows configuring limits that apply to the whole instance, and
> means that the whole instance can be reliably killed.  Services spawned
> by the instance will be in their own sub-cgroups, so one can apply
> separate limits to them so long as those limits do not exceed those
> of the containing cgroup.
>
> The hierarchy works like this:
>
> /sys/fs/cgroup
>   service1.service
>     @inner.service # processes go here
>     service2.service
>       @inner.service # and here
>       service3.service
>         @inner.service # and here
>
> You can apply resource limits at any level of the hierarchy.  Limits in
> nested cgroups can be smaller but not larger.  Terminating processes
> in a cgroup with cgroup.kill also terminates all processes in child
> cgroups.  After terminating the processes in a cgroup, cgroup-setup
> will delete everything in the hierarchy recursively.

I just don't really understand when we'd want that.  Why would we want
to limit all instances of vm-services, or all instances of serial-getty?

>>> diff --git a/tools/cgroup-setup/src/cgroup.rs b/tools/cgroup-setup/src/cgroup.rs
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..cd77becc0b40bfdcd983e53b63b7c21e4308d5fc
>>> --- /dev/null
>>> +++ b/tools/cgroup-setup/src/cgroup.rs
>>> @@ -0,0 +1,181 @@
>>> +// SPDX-License-Identifier: EUPL-1.2+
>>> +// SPDX-FileCopyrightText: 2026 Demi Marie Obenour <demiobenour@gmail.com>
>>> +
>>> +use std::fs::File;
>>> +use std::io::{Read, Seek};
>>> +use std::os::fd::{AsFd, OwnedFd};
>>> +use std::os::fd::{AsRawFd, BorrowedFd};
>>> +
>>> +use std::path::{Path, PathBuf};
>>> +
>>> +use rustix::fs::AtFlags;
>>> +use rustix::path::Arg;
>>> +use rustix::{
>>> +    fs::{Mode, OFlags, ResolveFlags},
>>> +    io::Errno,
>>> +};
>>> +
>>> +pub(crate) struct LeafCgroup {
>>> +    path: PathBuf,
>>> +    fd: OwnedFd,
>> 
>> root_fd would be a clearer name, if I'm understanding correctly.
>
> It's the FD for the cgroup directory, but not for the root of the
> cgroup tree.

But if you open a child cgroup, it's the same FD, right?  So it's a root
of something.

>>> +}
>>> +
>>> +enum Access {
>>> +    Read,
>>> +    Write,
>>> +}
>> 
>> Given this isn't public, it doesn't seem to add any value over passing
>> around OFlags::RDONLY and OFlags::WRONLY directly.
>
> Will change in v3.
>
>>> +
>>> +impl LeafCgroup {
>>> +    fn open_subtree(&self, path: &std::path::Path, access: Access) -> Result<OwnedFd, Errno> {
>>> +        rustix::fs::openat2(
>>> +            self.fd.as_fd(),
>>> +            path,
>>> +            OFlags::NOATIME
>>> +                | OFlags::CLOEXEC
>>> +                | OFlags::NOFOLLOW
>>> +                | match access {
>>> +                    Access::Write => OFlags::WRONLY,
>>> +                    Access::Read => OFlags::RDONLY,
>>> +                },
>>> +            Mode::empty(),
>>> +            ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFlags::NO_XDEV,
>>> +        )
>>> +    }
>>> +
>>> +    pub fn kill_processes(&self) -> std::io::Result<()> {
>>> +        let kill_fd = self.open_subtree(std::path::Path::new("cgroup.kill"), Access::Write)?;
>>> +        let wait_file = self.open_subtree(std::path::Path::new("cgroup.events"), Access::Read)?;
>>> +        let poll_fd = wait_file.as_raw_fd();
>>> +        let mut wait_fd = File::from(wait_file);
>>> +        assert_eq!(rustix::io::write(kill_fd.as_fd(), b"1")?, 1, "kernel bug");
>>> +        let mut fds = libc::pollfd {
>>> +            fd: poll_fd,
>>> +            events: libc::POLLIN | libc::POLLPRI | libc::POLLRDHUP,
>>> +            revents: 0,
>>> +        };
>>> +        // SAFETY: FFI call, valid arguments, fds contains 1 element
>>> +        let mut v = vec![];
>>> +        'a: loop {
>>> +            v.clear();
>>> +            if unsafe { libc::poll(&raw mut fds, 1, -1) } != 1 {
>>> +                panic!("poll failed");
>>> +            }
>> 
>> Shouldn't we start polling before writing?  Otherwise we might miss the
>> notification, if it comes in between the write and the poll.
>
> I'll test this, but I think it shouldn't matter.  If it does matter, I'll
> need to switch to epoll (or io_uring, but that's overkill here).

Not sure it's something you'll be able to reproduce in a test.  We'd
need to understand the reason it could never happen.

>>> +            wait_fd
>>> +                .seek(std::io::SeekFrom::Start(0))
>>> +                .expect("Seek on control group file should succeed");
>>> +            wait_fd
>>> +                .read_to_end(&mut v)
>>> +                .expect("reading from control group should work");
>>> +            for substr in v.split(|&c| c == b'\n') {
>>> +                if substr == b"populated 0" {
>> 
>> Could do this in one line, and avoid the labelled break:
>> 
>> if v.split(|&c| c == b'\n').any(|line| line == b"populated 0") {
>> 
>> (I think "line" is a clearer name than "substr".)
>
> Will fix in v3.
>
>>> +                    break 'a;
>>> +                }
>>> +            }
>>> +        }
>>> +        Ok(())
>>> +    }
>>> +}
>>> +
>>> +impl AsFd for LeafCgroup {
>>> +    fn as_fd(&self) -> std::os::fd::BorrowedFd<'_> {
>>> +        self.fd.as_fd()
>>> +    }
>>> +}
>>> +
>>> +impl LeafCgroup {
>>> +    pub(crate) fn open_cgroup_at(&self, p: &Path) -> Result<Self, Errno> {
>>> +        let fd = rustix::fs::openat2(
>>> +            self.fd.as_fd(),
>>> +            p,
>>> +            OFlags::NOATIME
>>> +                | OFlags::CLOEXEC
>>> +                | OFlags::NOFOLLOW
>>> +                | OFlags::RDONLY
>>> +                | OFlags::DIRECTORY,
>>> +            Mode::empty(),
>>> +            ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFlags::NO_XDEV,
>>> +        )?;
>>> +        Ok(Self {
>>> +            fd,
>>> +            path: self.path.join(p),
>>> +        })
>>> +    }
>>> +    pub(crate) fn open_cgroup(fd: BorrowedFd, p: &Path) -> Result<Self, Errno> {
>>> +        let fd = rustix::fs::openat2(
>>> +            fd,
>>> +            p,
>>> +            OFlags::NOATIME
>>> +                | OFlags::CLOEXEC
>>> +                | OFlags::NOFOLLOW
>>> +                | OFlags::RDONLY
>>> +                | OFlags::DIRECTORY,
>>> +            Mode::empty(),
>>> +            //ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFlags::NO_XDEV,
>> 
>> Stray comment.
>
> Will fix in v3.
>
>>> +            ResolveFlags::NO_SYMLINKS,
>>> +        )?;
>>> +        Ok(Self {
>>> +            fd,
>>> +            path: p.to_owned(),
>>> +        })
>>> +    }
>>> +    pub(crate) fn make_child(&self, p: &Path) -> Result<(), Errno> {
>>> +        rustix::fs::mkdirat(
>>> +            self.fd.as_fd(),
>>> +            p,
>>> +            Mode::RUSR
>>> +                | Mode::WUSR
>>> +                | Mode::XUSR
>>> +                | Mode::RGRP
>>> +                | Mode::XGRP
>>> +                | Mode::ROTH
>>> +                | Mode::XOTH,
>>> +        )
>>> +    }
>>> +
>>> +    pub(crate) fn delete_child(&self, path: &Path) -> Result<(), Errno> {
>>> +        remove_all(100, self.as_fd(), &path.as_cow_c_str().unwrap())
>>> +    }
>>> +}
>>> +
>>> +fn remove_recursively(fd: OwnedFd, remaining_depth: usize) -> Result<(), Errno> {
>>> +    if remaining_depth < 1 {
>>> +        panic!("control groups too deeply nested");
>>> +    }
>>> +    let mut d = rustix::fs::Dir::new(fd).expect("cannot start iterating");
>>> +    while let Some(element) = d.next() {
>>> +        let element = element.expect("Iterating through a cgroup directory failed?");
>>> +        if element.file_type() != rustix::fs::FileType::Directory {
>>> +            continue;
>>> +        }
>>> +
>>> +        let remaining_depth = remaining_depth - 1;
>>> +        let d: &rustix::fs::Dir = &d;
>>> +        let dirfd = d.fd().unwrap();
>>> +        let path = element.file_name();
>>> +        remove_all(remaining_depth, dirfd, path)?;
>>> +    }
>>> +    Ok(())
>>> +}
>>> +
>>> +fn remove_all(
>>> +    remaining_depth: usize,
>>> +    dirfd: BorrowedFd<'_>,
>>> +    path: &std::ffi::CStr,
>>> +) -> Result<(), Errno> {
>>> +    if path == c"." || path == c".." {
>>> +        return Ok(());
>>> +    }
>>> +    if rustix::fs::unlinkat(dirfd, path, AtFlags::REMOVEDIR).is_ok() {
>>> +        return Ok(());
>>> +    }
>>> +    let fd = rustix::fs::openat2(
>>> +        dirfd,
>>> +        path,
>>> +        OFlags::NOATIME | OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::RDONLY | OFlags::DIRECTORY,
>>> +        Mode::empty(),
>>> +        ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFlags::NO_XDEV,
>>> +    )?;
>>> +    remove_recursively(fd, remaining_depth)?;
>>> +    rustix::fs::unlinkat(dirfd, path, AtFlags::REMOVEDIR)?;
>>> +    Ok(())
>>> +}
>>> diff --git a/tools/cgroup-setup/src/main.rs b/tools/cgroup-setup/src/main.rs
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..358703a73fcbdc38794312fe3987e733f37c2df0
>>> --- /dev/null
>>> +++ b/tools/cgroup-setup/src/main.rs
>>> @@ -0,0 +1,196 @@
>>> +// SPDX-License-Identifier: EUPL-1.2+
>>> +// SPDX-FileCopyrightText: 2026 Demi Marie Obenour <demiobenour@gmail.com>
>>> +
>>> +use std::{
>>> +    ffi::{OsStr, OsString},
>>> +    fs::File,
>>> +    io::Write as _,
>>> +    os::unix::prelude::*,
>>> +    path::{Component, Path, PathBuf},
>>> +};
>>> +
>>> +use rustix::{
>>> +    fs::{FlockOperation, Mode, OFlags, ResolveFlags},
>>> +    io::Errno,
>>> +};
>>> +
>>> +mod cgroup;
>>> +
>>> +macro_rules! fail {
>>> +    ($($arg:tt)*) => {{
>>> +        eprintln!($($arg)*);
>>> +        std::process::exit(1)}
>>> +    };
>>> +}
>> 
>> I'd find the error handling Rustier if we did error returns with the ?
>> operator, like how mount-flatpak does it.  The error type can just be
>> String.
>
> Sure!
>
>>> +
>>> +fn main() {
>>> +    let mut args = std::env::args_os();
>>> +    let Some(prog_name) = args.next() else {
>>> +        fail!("No command line arguments (argv[0] is NULL)");
>>> +    };
>>> +    let mut cgroup_relative_path = args.next();
>>> +    if cgroup_relative_path.as_deref() == Some(OsStr::from_bytes(b"--")) {
>>> +        cgroup_relative_path = args.next();
>>> +    }
>> 
>> We could just not support -- and simplify invocation, right?
>
> Only if we never want to support any options in the future.

I guess it's nice for out-of-tree users, and it's a trivial amount of
code.

>>> +
>>> +    let Some(cgroup_relative_path) = cgroup_relative_path else {
>>> +        fail!("{prog_name:?}: Have no positional arguments, expect at least 1")
>>> +    };
>>> +    let mut cgroup_relative_path = cgroup_relative_path.into_vec();
>>> +
>>> +    // slow but we do not care
>> 
>> Slow?
>
> O(n) for a size-n Vec.  Not something you want in a hot path, but fine here.

How is getting the length of a Vec O(n)?  It stores it.

>>> +    if cgroup_relative_path.len() > 247 {
>> 
>> This magic number could perhaps use a name.
>
> Sure.
>
>>> +        fail!("{prog_name:?}: Cgroup name {cgroup_relative_path:?} too long");
>>> +    }
>>> +    if cgroup_relative_path.is_empty() {
>>> +        fail!("{prog_name:?}: Cgroup name {cgroup_relative_path:?} empty");
>>> +    }
>>> +    if cgroup_relative_path[0] == b'-' {
>>> +        fail!("{prog_name:?}: Cgroup name {cgroup_relative_path:?} starts with '-'");
>>> +    }
>> 
>> Is that a problem?
>
> In theory, no, but it's probably a bug in the caller.
>
>>> +    for &i in &cgroup_relative_path {
>>> +        match i {
>>> +            b'/' => fail!(
>>> +                "{prog_name:?}: Cgroup name {cgroup_relative_path:?} \
>>> +contains /"
>>> +            ),
>>> +            b'$' => fail!(
>>> +                "{prog_name:?}: Cgroup name {cgroup_relative_path:?} \
>>> +contains $: did you forget an execline substitution?"
>>> +            ),
>>> +            b'!'..=b'~' => {}
>>> +            _ => fail!(
>>> +                "{prog_name:?}: Cgroup name {cgroup_relative_path:?} \
>>> +contains space, non-ASCII character, or control character (byte {i})"
>>> +            ),
>> 
>> Surely every kernel interface we're going to use is 8-bit clean.  If
>> this is about log viewing again, I really don't like it.  Even if we're
>> careful about it in first-party code (which itself is a big ask), we
>> can't expect it of every other package that might log something.  People
>> viewing logs have to use robust tools to do so regardless, or we have to
>> mitigate this somewhere centrally, e.g. in s6 log.  Doing this piecemeal
>> does not meaningfully mitigate the problem, and arguably creates a false
>> sense of security.
>
> This was inspired by systemd, which has restrictions on what characters
> are allowed in service names.  Also, restricting allowed characters
> makes it easier to find bugs, such as "${a} ${b}" instead of "${a}"
> "${b}" in an execline script.
>
> Forbidding '$' is very much intentional, and is inspired by me making
> the exact mistake described by the error message.  It also means that
> certain names can be reserved for internal use.

In my opinion, one component should not be responsible for ad-hoc bug
checks in another, but I won't push back too much.  The other character
limitations seem completely arbitrary to me though.

>>> +        }
>>> +    }
>>> +    cgroup_relative_path.extend_from_slice(b".service");
>> 
>> What do we need the suffix for?
>
> The kernel may add new control files at any time.  However, it will
> never use the .service suffix to avoid breaking systemd.  Therefore, I
> chose to use that suffix to avoid breaking in future kernel releases.

Makes sense.

>>> +    let cgroup_relative_path = Path::new(OsStr::from_bytes(&cgroup_relative_path));
>>> +
>>> +    let local_cgroup = std::fs::read("/proc/thread-self/cgroup")
>> 
>> Ooc, why /proc/thread-self instead of /proc/self?
>
> Why not?  The two are aliases in this case, but in general /proc/thread-self is likely the better choice.

I'd just never seen it before.

>>> +        .unwrap_or_else(|e| fail!("{prog_name:?}: cannot read /proc/thread-self/cgroup: {e}"));
>>> +
>>> +    if !local_cgroup.starts_with(b"0::/")
>>> +        || !local_cgroup.ends_with(b"\n")
>>> +        || local_cgroup.contains(&b'\0')
>>> +    {
>>> +        fail!("{prog_name:?}: Kernel bug: Bad contents of /proc/thread-self/cgroup");
>>> +    }
>> 
>> We are not a kernel fuzzer.  We can't reasonably write programs that
>> have to anticipate kernel contract violations.
>
> Fair!
>
>>> +
>>> +    let mut local_cgroup = PathBuf::from(<OsString as OsStringExt>::from_vec(
>>> +        local_cgroup[4..local_cgroup.len() - 1].to_owned(),
>> 
>> … but maybe we could make this clearer, and still satisfy your instincts
>> to validate, by using slice::strip_prefix and slice::strip_suffix,
>> unwrapping the results?
>
> That's even better, because it's much easier to read than magic numbers.
>
>>> +    ));
>>> +
>>> +    match local_cgroup.components().next_back() {
>>> +        Some(Component::Prefix(_)) => unreachable!("does not occur on Linux"),
>>> +        Some(Component::RootDir) | None => {}
>>> +        Some(Component::CurDir) => unreachable!("not produced by kernel"),
>>> +        Some(Component::ParentDir) => unreachable!("not produced by kernel"),
>>> +        Some(Component::Normal(os_str)) => {
>>> +            if os_str.as_bytes() == b"@inner.service" {
>>> +                let _ = local_cgroup.pop();
>>> +            }
>>> +        }
>>> +    }
>> 
>> Could we not simplify this with local_cgroup.file_name()?  If it's None,
>> it's the root directory; otherwise it's the equivalent of normal.
>
> Yup!
>
>>> +
>>> +    if local_cgroup.as_os_str().is_empty() {
>>> +        local_cgroup = PathBuf::from(".");
>>> +    }
>> 
>> Path::is_empty is stable in 1.98.  Could we mention that in a TODO/FIXME
>> comment so we can use it when available?
>
> Will do.
>
>>> +    let cgroup_root = rustix::fs::openat2(
>>> +        rustix::fs::CWD,
>>> +        Path::new("/sys/fs/cgroup"),
>>> +        OFlags::DIRECTORY | OFlags::RDONLY | OFlags::CLOEXEC | OFlags::NOFOLLOW,
>>> +        Mode::empty(),
>>> +        ResolveFlags::NO_MAGICLINKS | ResolveFlags::NO_SYMLINKS,
>>> +    )
>>> +    .unwrap_or_else(|e| fail!("{prog_name:?}: cannot open /sys/fs/cgroup: {e}"));
>> 
>> Why do we need to use openat2 for this, but not for reading
>> /proc/thread-self/cgroup?  I'm a bit surprised to see openat2 here at
>> all, since only root could manipulate these paths, in which case they
>> have no need to use this program as a confused deputy, right?
>> 
>> If openat2 is to stay, isn't OFlags::NOFOLLOW redundant with
>> ResolveFlags::NO_SYMLINKS?
>
> openat2() is indeed pointless here.

Great — I think we can really simplify the code if we don't use it where
unnecessary, because we can use a lot more of the standard library.
Possibly Rustix isn't even needed?

>>> +
>>> +    let cgroup = match cgroup::LeafCgroup::open_cgroup(cgroup_root.as_fd(), &local_cgroup) {
>>> +        Ok(e) => e,
>>> +        Err(e) => fail!(
>>> +            "{prog_name:?}: Failed to open {}: {e:?}",
>>> +            local_cgroup.display()
>>> +        ),
>>> +    };
>>> +
>>> +    match rustix::fs::flock(cgroup.as_fd(), FlockOperation::LockExclusive) {
>>> +        Ok(()) => {}
>>> +        Err(e) => fail!("{prog_name:?}: cannot lock cgroup: {e}"),
>>> +    }
>> 
>> Would std::fs::File::lock not be a bit Rustier?
>
> Yes :).  I know how to 
> 	
>>> +    purge_cgroup(&prog_name, &cgroup, cgroup_relative_path);
>> 
>> I find it a little weird that we run this program in both service
>> startup and finish, when there are really two completely separable parts
>> to it, right?  Why is purging not a separate program that only runs in
>> finish?
>
> The finish script might fail, time out, etc.  Purging at startup
> ensures that any previous cgroup state (like attached BPF programs)
> is removed, and it ensures that the program this program invokes will
> not run concurrently.  It's just more robust overall.
>
> Purging is idempotent, and purging an already-purged cgroup is very
> cheap.  Also, this allows using cgroup-setup without a finish script,
> while still ensuring that if the service restarts there are no stale
> processes left behind.

Alright.

>>> +    let Some(program_name) = args.next() else {
>>> +        return;
>>> +    };
>> 
>> This could be moved closer to where it's used.
>
> Will fix in v3.
>
>>> +    if let Err(e) = cgroup.make_child(cgroup_relative_path) {
>>> +        fail!("{prog_name:?}: cannot create child cgroup: {e}")
>>> +    }
>>> +    let child = cgroup
>>> +        .open_cgroup_at(cgroup_relative_path)
>>> +        .unwrap_or_else(|e| {
>>> +            fail!("{prog_name:?}: cannot create child cgroup {cgroup_relative_path:?}: {e}")
>>> +        });
>>> +    child
>>> +        .make_child(Path::new("@inner.service"))
>>> +        .unwrap_or_else(|e| {
>>> +            fail!("{prog_name:?}: cannot create child cgroup {cgroup_relative_path:?}/@inner.service: {e}")
>>> +        });
>>> +    let child = child
>>> +        .open_cgroup_at(Path::new("@inner.service"))
>>> +        .unwrap_or_else(|e| {
>>> +            fail!("{prog_name:?}: cannot open child cgroup {cgroup_relative_path:?}/@inner.service: {e}")
>>> +        });
>> 
>> This would really benefit from having been written up, so I don't have
>> to guess what it's for.  We only really need it for supervisors, right?
>
> It's only *needed* when the program might be called recursively,
> but it's easier to just use it everywhere.

Yeah, that makes sense.  Would just be nice to have that documented
somewhere so the next person doesn't have to spend the time I did
figuring it out!

>>> +
>>> +    // SAFETY: safe FFI call
>>> +    let pid = unsafe { libc::getpid() };
>> 
>> Not std::process::id()?
>
> Only because I was not aware of it.
>
>>> +    write_cgroup_value(&prog_name, &child, "cgroup.procs", &pid.to_string());
>>> +
>>> +    let e = std::process::Command::new(&program_name).args(args).exec();
>>> +    fail!(
>>> +        "{prog_name:?}: cannot spawn child {:?}: {}",
>>> +        program_name,
>>> +        e
>>> +    );
>>> +}
>>> +
>>> +fn write_cgroup_value(prog_name: &OsStr, child: &cgroup::LeafCgroup, name: &str, value: &str) {
>> 
>> Perhaps it would be nicer for this to be a method on LeafCgroup?
>
> Sure!
>
>>> +    let path = Path::new(name);
>>> +    let fd = rustix::fs::openat2(
>>> +        child.as_fd(),
>>> +        path,
>>> +        OFlags::NOATIME | OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::WRONLY,
>>> +        Mode::empty(),
>>> +        ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFlags::NO_XDEV,
>>> +    )
>>> +    .unwrap_or_else(|e| fail!("{prog_name:?}: cannot open {:?}: {}", path, e));
>>> +    let () = File::from(fd)
>>> +        .write_all(value.as_bytes())
>>> +        .unwrap_or_else(|e| {
>>> +            fail!(
>>> +                "{prog_name:?}: Cannot write {:?} to {:?}: {}",
>>> +                name,
>>> +                path,
>>> +                e
>>> +            )
>>> +        });
>> 
>> Similarly to above, if we don't need openat2, this could be done more
>> nicely as std::fs::write.
>
> std::fs doesn't even expose openat(), and I'm much more comfortable
> using a directory FD here.  This is a fairly severe limitation in
> the stdlib.  Fixing it requires using the Native API on Windows,
> but that's easy enough.

We have a lock on the cgroup though.  What are we protecting against by
using openat?

>>> +}
>>> +
>>> +fn purge_cgroup(prog_name: &OsStr, cgroup: &cgroup::LeafCgroup, cgroup_relative_path: &Path) {
>>> +    let child = match cgroup.open_cgroup_at(cgroup_relative_path) {
>>> +        Ok(child_cgroup) => child_cgroup,
>>> +        Err(Errno::NOENT) => return,
>>> +        Err(other) => {
>>> +            fail!("{prog_name:?}: cannot open child cgroup {cgroup_relative_path:?}: {other}")
>>> +        }
>>> +    };
>>> +    child.kill_processes().unwrap_or_else(|e| {
>>> +        fail!("{prog_name:?}: cannot kill programs in {cgroup_relative_path:?}: {e}")
>>> +    });
>>> +
>>> +    cgroup
>>> +        .delete_child(cgroup_relative_path)
>>> +        .unwrap_or_else(|e| {
>>> +            fail!("{prog_name:?}: delete child cgroup {cgroup_relative_path:?}: {e}")
>>> +        });
>>> +}

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]