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

[Demi Marie Obenour <demiobenour@gmail.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 25750 bytes --]

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.

>> diff --git a/host/rootfs/image/etc/fstab b/host/rootfs/image/etc/fstab
>> index 18bb5e45abafeaf43871306ce8233239e5c07f76..d92364d83f4d26f766fcd5b494614c06a630d2fe 100644
>> --- a/host/rootfs/image/etc/fstab
>> +++ b/host/rootfs/image/etc/fstab
>> @@ -6,3 +6,4 @@ tmpfs	/dev/shm	tmpfs	nosuid,nodev					0	0
>>  tmpfs	/media		tmpfs	nosuid,nodev,noexec,nosymfollow,mode=755	0	0
>>  sysfs	/sys		sysfs	nosuid,nodev,noexec				0	0
>>  tmpfs	/tmp		tmpfs	nosuid,nodev					0	0
>> +cgroup2 /sys/fs/cgroup  cgroup2 nosuid,nodev,noexec,nosymfollow			0	0
> 
> Alignment is off here.  The rest of the file uses tabs at 8 characters.
> (I know, I know, but it's traditional.)
> 
>> diff --git a/host/rootfs/image/etc/s6-linux-init/run-image/service/serial-getty-generator/run b/host/rootfs/image/etc/s6-linux-init/run-image/service/serial-getty-generator/run
>> index bf66ab0878fc6d8e77ca71a4a89e50c139a6bb11..5c9988c994ed9b942329055673b332d0aa918957 100755
>> --- a/host/rootfs/image/etc/s6-linux-init/run-image/service/serial-getty-generator/run
>> +++ b/host/rootfs/image/etc/s6-linux-init/run-image/service/serial-getty-generator/run
>> @@ -1,7 +1,8 @@
>> -#!/bin/execlineb -WP
>> +#!/bin/execlineb -WS1
>>  # SPDX-License-Identifier: EUPL-1.2+
>>  # SPDX-FileCopyrightText: 2024-2025 Alyssa Ross <hi@alyssa.is>
>>  
>> +cgroup-setup -- $1
>>  piperw 3 4
>>  background {
>>    fdclose 3
> 
> What benefit does this cgroup provide?

None, I'll remove it.

The only requirement of the current code is that if a service is under
a cgroup, its parent should *also* be under a cgroup.  Otherwise you
can get conflicting cgroup paths.

>> 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.

>> diff --git a/tools/cgroup-setup/Cargo.toml b/tools/cgroup-setup/Cargo.toml
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..9cce0be706151dfd5264a81afb3ee9a6c851367c
>> --- /dev/null
>> +++ b/tools/cgroup-setup/Cargo.toml
>> @@ -0,0 +1,17 @@
>> +# SPDX-License-Identifier: CC0-1.0
>> +# SPDX-FileCopyrightText: 2025 Alyssa Ross <hi@alyssa.is>
>> +# SPDX-FileCopyrightText: 2026 Demi Marie Obenour <demiobenour@gmail.com>
>> +
>> +[package]
>> +name = "cgroup-setup"
>> +edition = "2024"
>> +
>> +[dependencies]
>> +libc = "0.2.177"
>> +rustix = { version = "1.1.2", features = ["fs"] }
>> +
>> +[profile.release]
>> +split-debuginfo = "symbols"
>> +strip = "symbols"
>> +lto = true
>> +panic = "abort"
> 
> What's with all this stuff?  Why do we set it only for this program, and
> not for any others?  Please don't introduce divergence in build settings
> between programs without a good reason.

I was trying to reduce the binary size as it seemed rather bloated.
I'll leave them out 

>> 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.

>> +}
>> +
>> +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).

>> +            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.

>> +
>> +    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.

>> +    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.

>> +        }
>> +    }
>> +    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.

>> +    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.

>> +        .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.

>> +
>> +    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.

>> +    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.

>> +
>> +    // 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.

>> +}
>> +
>> +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}")
>> +        });
>> +}


-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7253 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]