Add reboot syscall

[puppymati]

This pr tries to implement a reboot syscall. Fixes #826.

Unfortunately I hit a roadblock so I'm opening a draft pr in hope of resolving it. To implement reboot we also need some constants defined in reboot.h. This are supposedly c_int (or i32) but the rust compiler tells me their out of range and refuses to compile them:

error: literal out of range for `i32`
   --> src/backend/linux_raw/c.rs:214:47
    |
214 | pub(crate) const LINUX_REBOOT_MAGIC1: c_int = 0xfee1dead;
    |                                               ^^^^^^^^^^
    |
    = note: the literal `0xfee1dead` (decimal `4276215469`) does not fit into the type `i32` and will become `-18751827i32`
    = help: consider using the type `u32` instead
    = note: `#[deny(overflowing_literals)]` on by default
help: to use as a negative number (decimal `-18751827`), consider using the type `u32` for the literal and cast it to `i32`
    |
214 | pub(crate) const LINUX_REBOOT_MAGIC1: c_int = 0xfee1deadu32 as i32;

Perhaps I'm missing something obvious since the libc crate also defines them as c_int and compiles without issues. Any help would be appreciated.

[sunfishcode]

The libc crate enables allow(overflowing_literals), which is what allows it to avoid that error.

For rustix, it'd be ok to declare those constants as u32 instead of c_int, and to change the parameter types of the reboot function to u32 too. Alternatively, it'd also be ok to just cast the constants to c_int using the bitcast! macro.

Also, reboot should go in the system module (src/system, src/backend/*/system) instead of the process module, since a reboot affects the entire system.

[puppymati]

The libc crate enables allow(overflowing_literals), which is what allows it to avoid that error.

Feels like we should do the same but localized for that module. Otherwise we risk diverging from the behavior of the libc backend

Also, reboot should go in the system module (src/system, src/backend/*/system) instead of the process module, since a reboot affects the entire system.

I wasn't actually sure about that myself but I'll move it there then

[puppymati]

I moved everything to system and properly documented everything. There are only a couple issues remaining:

• Testing. I am not sure how to implement tests for this syscall. Since it interacts directly with the system by halting, rebooting, etc., we probably want to test with qemu although I am not sure how exactly
• LINUX_REBOOT_CMD_RESTART2. This is the only command that allows passing an arg, unfortunately libc implementations seem to just ignore its existence and not allow any arg. We could diverge from that by using an Option but then the libc backend would not work properly. I am actually not sure if there is a reason to ignore it or if it's just ignored for historical reasons.

[sunfishcode]

On testing, I'd say it's enough to add a test that checks if the process has CAP_SYS_BOOT and if not, calls reboot and asserts that it gets Errno::PERM is sufficient for now.

On LINUX_REBOOT_CMD_RESTART2, my thinking is, this has been in Linux since 2.1.30, and if no one has added a way to use it from libc after all this time, then it's probably not widely used, and we can ignore it too, at least for now.

[puppymati]

On testing, I'd say it's enough to add a test that checks if the process has CAP_SYS_BOOT and if not, calls reboot and asserts that it gets Errno::PERM is sufficient for now.

That seems reasonable, I won't be able to work on this until monday so if someone else wants to tackle it that'd be swell

On LINUX_REBOOT_CMD_RESTART2, my thinking is, this has been in Linux since 2.1.30, and if no one has added a way to use it from libc after all this time, then it's probably not widely used, and we can ignore it too, at least for now.

That's what I thought too but at the same time, it might be a chicken and egg problem where no one uses it because it's not implemented and no one implements it because it's not used. It's probably fine to ignore for now but it does make me wonder if rustix should provide an api that diverges from libc in some cases. I am not sure a project like this should bear the burden of the historical issues that plague libc/posix

[sunfishcode]

On LINUX_REBOOT_CMD_RESTART2, my thinking is, this has been in Linux since 2.1.30, and if no one has added a way to use it from libc after all this time, then it's probably not widely used, and we can ignore it too, at least for now.

That's what I thought too but at the same time, it might be a chicken and egg problem where no one uses it because it's not implemented and no one implements it because it's not used. It's probably fine to ignore for now but it does make me wonder if rustix should provide an api that diverges from libc in some cases. I am not sure a project like this should bear the burden of the historical issues that plague libc/posix

If you are specifically planning to use LINUX_REBOOT_CMD_RESTART2, we can have the libc backend call it through libc::syscall, using the syscall! macro. That said, if you aren't planning to use it, my guess is it's not important enough to add it speculatively.

[puppymati]

If you are specifically planning to use LINUX_REBOOT_CMD_RESTART2, we can have the libc backend call it through libc::syscall, using the syscall! macro. That said, if you aren't planning to use it, my guess is it's not important enough to add it speculatively.

At first I was planning on using it but I am wondering if there is a technical reason to avoid it. It'd probably be trivial to implement it if necessary so I'll open another pr if needed

[sunfishcode]

The Linux shutdown command has its own logic for logging messages to syslog and writing to all user's terminals, and that it needs that logic in any case because it also handles messages like "system will shutdown in 5 minutes" which LINUX_REBOOT_CMD_RESTART2 wouldn't be able to do. I don't know the history, but perhaps LINUX_REBOOT_CMD_RESTART2 was just redundant with this code that shutdown needed anyway. And from a Unix-philosophy perspective, it kinda does make sense that reboot should just do the reboot/shutdown/suspend things, and not also add its own ad-hoc and limited message printing facility.

I'm ok either way. We can add it, and use the syscall! macro in the libc backend, or we can leave it out. I have a general preference for omitting features that have no known use cases, but if you have a use case, then it's fine.

[puppymati]

The Linux shutdown command has its own logic for logging messages to syslog and writing to all user's terminals, and that it needs that logic in any case because it also handles messages like "system will shutdown in 5 minutes" which LINUX_REBOOT_CMD_RESTART2 wouldn't be able to do. I don't know the history, but perhaps LINUX_REBOOT_CMD_RESTART2 was just redundant with this code that shutdown needed anyway. And from a Unix-philosophy perspective, it kinda does make sense that reboot should just do the reboot/shutdown/suspend things, and not also add its own ad-hoc and limited message printing facility.

LINUX_REBOOT_CMD_RESTART does also print a message so there is some kind of logging facility, I'm not sure that's the reason

I'm ok either way. We can add it, and use the syscall! macro in the libc backend, or we can leave it out. I have a general preference for omitting features that have no known use cases, but if you have a use case, then it's fine.

I do not think it's a good idea to add it right now. I am not sure if I have a use case yet and I wouldn't want to implement something that's never used or worst case, shouldn't be used. I will investigate further the history of this command to see if it makes sense and open an appropiate issue then. If it turns out it's "bad" for some reason it might even be worth removing it entirely. I do actually wonder if we should include a warning of some kind or hide/remove it for now since it can't actually be called and will always cause an error if used.

[sunfishcode]

Yeah, if we don't add a way to pass the argument, we should remove Restart2 entirely.

[puppymati]

Yeah, if we don't add a way to pass the argument, we should remove Restart2 entirely.

I don't know if we should outright remove it. If we do then adding it back in the future for any reason would need a semver bump. Unless we mark the enum as non_exaustive, which might actually be a good idea regardless as it would future proof rustix in the unlikely event that the kernel adds more commands.

[sunfishcode]

Marking it non_exhaustive makes sense.

[puppymati]

I have added a test as discussed above, it should work fine but I think it might be worth thinking about a more thorough test suite in the future. I have also marked the enum as non_exhaustive and hidden the Restart2 variant.

I think it should be ready to merge.

[sunfishcode]

My general goal for rustix is that we should either write documentation ourself, or link to Linux's documentation, but not copy and paste whole paragraphs from Linux's documentation. If we're copying and pasting, developers would often be better off referring to Linux's documentation, which often has more context. In this case, Linux's documentation includes context like what "CAD" stands for, and the fact that the key sequence can be changed.

I know that having docs in Rust comments is more convenient for Rust developers. On the other hand, this is reboot and it's not something anyone is going to be using on a regular basis, and it has some subtle behavior not included here, so it really would be better to encourage developers to read the Linux docs directly, rather than copying and pasting parts of them here.

[puppymati]

What would you recommend we do then? I copied and pasted it because I felt like it made sense but I can see it is missing a bunch of context. Maybe we can just not document it and refer user to the man page, something like reboot should probably not be touched by someone who hasn't read it.

[sunfishcode]

Yeah, I'd say leave most of the text out, perhaps just add short one-line descriptions if anything, and add a link to the Linux docs in RebootCommand's doc comment to help people find it.

[puppymati]

I updated the docs the reboot function to be more clear about its usage. I also changed the RebootCommand docs to be a bit more friendly instead of just being copy-pasted from the man page. Now the ci is failing but I could swear I only update the docs unless I'm missing something

[sunfishcode]

Could you rebase this on origin/main? I expect that'll clear up the CI failures.

[puppymati]

Could you rebase this on origin/main? I expect that'll clear up the CI failures.

Doesn't look like it worked. However I just realized that the latest commit is actually failing on main 707c51e

[sunfishcode]

I think the problem was a stale cache of the qemu build. I've now removed the old cache; could you try again?

[puppymati]

I think the problem was a stale cache of the qemu build. I've now removed the old cache; could you try again?

I think that solved the problem

[sunfishcode]

Looks good!

[sunfishcode]

This is now released in rustix 0.38.17.