Add reboot() support on Linux

[bugaevc]

This adds support for calling reboot(2) on Linux (useful for implementing PID 1!).

• Testing: I can't imagine how this could be tested other than manually, which is what I did to ensure it works.
• libc: for some reason libc includes neither reboot() function, nor RB_* constants, so I had to hack-define them here. Should I open a bug/PR against the libc crate?
• API: I went with reboot() function for restarting, powering off, halting, etc. and an additional set_cad_enabled() function for enabling/disabling Ctrl-Alt-Delete, which still corresponds to reboot() C call, but has different semantics and return type. (But I'm open to suggestions here -- maybe it would be better to separate each action into its own function, i.e. restart(), poweroff(), halt()?)
• Documentation: I see most of the things in nix are undocumented, so I didn't document anything except for the set_cad_enabled() function, which obviously uses a non-standard name.
• Portability: I implemented the Linux version as I'm not familiar with anything else, but there surely are others (ex. BSD). If I didn't mess up anything, the new code is only compiled on Linux/Android, so it should be straightforward to implement the other versions too.

[bugaevc]

To test, as root:

extern crate nix;

use nix::sys::reboot::*;

fn main() {
    println!("Restarting");
    reboot(RebootMode::Restart).unwrap();
    unreachable!();
}

[fiveop]

Thank you for the pull request.

My understanding is, that we aim with nix to provide rust-style wrappers for rust-libc. So, I would prefer, if you could get all necessary ffi-bindings and constants into libc first. This has the advantage that libc has test infrastructure that ensures that bindings and constants exist and use the correct type.

In addition, we put our code in files with names corresponding to their C includes. According to http://man7.org/linux/man-pages/man2/reboot.2.html, your code should be in sys/reboot.rs instead of reboot.rs. (We need to put text detailing this into CONVENTIONS.md).

In general, you should have look at the CONVENTIONS.md file.

[bugaevc]

@fiveop

• I wouldn't dare making a pull request without reading CONTRIBUTING.md amd CONVENTIONS.md (and in case of nix also Figure out the future of nix #190). So yeah, I've seen it (quite a few times).
• The code is indeed in sys/reboot.rs -- or am I missing something, maybe you mean sys/sys/reboot.rs?
• I would prefer getting the prototypes into libc too; but I wonder whether it's not included there as of now for a reason -- or is it just a thing to fix.

[fiveop]

First of all, sorry about the sys/reboot.rs mixup. I made a mistake there.

Regarding your third point: If something from a libc-header or sys/ header is mising from libc, there is probably no other reason, than that no one has bothered to add it yet. Given that musl and glibc provide the reboot method as well as associated constants, that should be no problem.

[bugaevc]

Filed rust-lang/libc#333
uh-oh, a circular reference! ;)

[bugaevc]

So that got merged, now waiting for either #387 (or its analogue) to be accepted -- or a libc release.

[posborne]

Ok, #387 merged, so things should be ready for a refresh.

[bugaevc]

Great! Should I merge or rebase?

[bugaevc]

"Timed out waiting for reply" 😄
Rebasing...

[bugaevc]

OK, so:

• libc: now using libc's stuff!
• Portability: no longer targeting Android, because I wasn't interested enough to figure it out on the libc level. Shouldn't be hard to fix though, if anyone needs it.

Also, I went with repr(i32) where repr(libc::c_int) would be better -- though it's not an ABI or anything, just needs to be a wide enough integer type that can represent the particular constants.

[bugaevc]

So i32 doesn't quite work -- let's make it just an enum instead and match on its values manually.

[fiveop]

Could you switch Clone and Copy? (I know it's pedantic, I'll provide more substantial feedback momentarily)

[bugaevc]

Sure. Is there a general convention about the preferred order of stuff in #derive? If so, maybe it should be mentioned/documented somewhere?

[fiveop]

I commented on your changes. If the comments are addressed - through discussion or commits - I would like to merge this pull request.

Thank you for the work.

[bugaevc]

Please feel free to merge this if you think I've addressed your comments -- and thanks for the review.

[fiveop]

(Somehow a comment went missing yesterday; I might have forgotten to press comment).

What problem did occur when you tried to use #[repr(i32)]? All constants have type libc::c_int which is an alias for i32. The enumeration Signal in sys/signal.rs does the same. You could then get rid of the match bellow.
As you can see there, I used the name of the corresponding constants for the elements of the enumeration. I would prefere if we did that here as well, even though we have no convention yet. However, bitflags types use this convention, so it would be consistent.

[fiveop]

Having looked at your commit history, your problem probably was that you still have to cast the enumeration elements to libc::c_int in order to pass them to reboot, i.e.

reboot(cmd as libc::c_int)

[bugaevc]

As you can see from this commit, that's exactly what I tried before, and here's the Travis failure that GitHub UI displays as a little red cross next to the commit -- however now that I look carefully I see that the commit hash is wrong and the link back gives me 404 -- is it a bug or something (I definetely commited that after rebasing)?

On your "middle" point -- do you mean to call the enum variants RebootMode::RB_POWER_OFF as so on? Doesn't seem beautiful; but if that's the convention -- OK ¯\_(ツ)_/¯. I would rather prefer to name them appropriately and list the C analogues in the documentation, but we obviously can't rename all of the bitflags now, so consistency wins, yeah.

[fiveop]

Regarding the names, you understood me correctly.

I had a look at the Travis output and I dont understand it. The constants are libc::c_int in libc, which is i32 hence an enumeration with #[repr(i32)] should work.

[bugaevc]

So maybe it is a bug in GH/Travis. Let me change the names and try #[repr(i32)] one more time then.

[bugaevc]

It's failing again...

[fiveop]

I'll have time to investigate this behavior on the weekend. If you're in a hurry, you have to look into it yourself. I just want to understand, why we can not go this route, if we really can not.

[bugaevc]

No, I'm unlikely to need this sooner than in a week, if ever.

I'd really appreciate any help with investigating this, because I don't see any pattern in the failures -- they happen across different architectures and compiler versions, while other builds in seemingly similar conditions pass.

[posborne]

I can help take a look as well as I will be wanting reboot for some work I am doing as well.

[bugaevc]

Dumb me -- there obviously is a pattern. Ignoring Mac builds as they don't compile the module, failures happen on -- and only on -- rusc versions older than "stable". Maybe there was a bug that got fixed?

[fiveop]

I looked into it. We can get rid of the error (and rid of similar warning in libc stable and younger) by declaring the constant literals to be u32 and casting them to i32. For example,

pub const FOO : i32 = 0x9000000u32 as i32;

I'll prepare a patch for libc.

[bugaevc]

Yeah, but the questions are:

• Why does this happen in the first place? Are the values really out-of-the-range for i32 (C's int)? As in, the Linux kernel mistakenly uses int, but it should have used unsigned int?
• Why do different versions of rustc treat it differently?

[posborne]

Are the values really out-of-the-range for i32

Yes-and-no. They fit when you interpret them as C does (if you printed the number with the cast to i32 you have a negative number).

As in, the Linux kernel mistakenly uses int, but it should have used unsigned int?

This is a system call. There aren't really real types associated at that point in time (the value could be a value, pointer, etc.). Way back in the day, libc probably should have defined the interface explicitly as u32 but they didn't. For C where the constants are defined using #define the user experience wasn't really any different (but causes some headaches for us now).

Why do different versions of rustc treat it differently?

I'm still wondering about that as well. @alexcrichton Any ideas?

[alexcrichton]

Why do different versions of rustc treat it differently?

We've had a number of changes to the constant folder and such recently, mostly in the area of fixing bugs but perhaps a few accidental corner cases were fixed only later? Other than that I'm also not sure what's happening :(

[bugaevc]

Not that this is all really important, but I'm just trying to better understand what's going on, so anyway:

This is a system call. There aren't really real types associated at that point in time (the value could be a value, pointer, etc.).

True, the value is "untyped" between libc and the syscall handler, but it's "typed" on both sides -- and I see no reason for the types to differ. I peeked into the kernel source and guess what -- it's indeed unsigned int cmd!

Way back in the day, libc probably should have defined the interface explicitly as u32 but they didn't.

I see it was added on 23 May 1996 (according to this), but all the mailing list archives I can find only go back to 1999 😢

They fit when you interpret them as C does (if you printed the number with the cast to i32 you have a negative number).

It's strange that the C compiler (at least gcc) doesn't warn you that the value overflows -- because it does so when you pass something really big, like int x = 0xD000FCE20000000; (saying "overflow in implicit constant conversion"). My guess would be that it means "unsigned overflow" (too many bits) as opposed to "signed overflow" (the sign changed).

For C where the constants are defined using #define the user experience wasn't really any different

It shouldn't matter whether the conversion from "the constant" to the actual type is performed -- at the definition or when passing arguments to the function -- either the value fits into i32/int according to the language rules or it doesn't.

---

Anyway, since rust-lang/libc#334 is merged (btw how come we didn't get the same warning/error there before?), this should be good to go now?

[bugaevc]

We've had a number of changes to the constant folder and such recently, mostly in the area of fixing bugs but perhaps a few accidental corner cases were fixed only later?

If building this fine (as opposed to failing with an error) on newer versions is a bug, then it's still not fixed -- the current nightly builds this fine (just checked).

I can help take a look as well as I will be wanting reboot for some work I am doing as well.

By any chance, are you working on something interesting [that you are allowed to talk about]? Am I wrong in assuming that reboot() shouldn't be used anywhere but in PID 1?

[fiveop]

The behavior in stable and newer versions is perfectly fine. You get a warning when declaring the constants. After that the constant has type i32 thus you should not get an error for the enumeration.

I restarted the CI.

[bugaevc]

You get a warning when declaring the constants.

Did it give a warning when the constants were added to libc? I don't remember getting one.

[fiveop]

They might have disabled it. I did get a warning, in my test app.

[fiveop]

@homu r+

[homu]

📌 Commit 215f387 has been approved by fiveop

[homu]

⚡ Test exempted - status

[fiveop]

Thank you for going through the (rather long) process bugaevc.

[bugaevc]

Great! Thank you for helping me get it done.

[posborne]

By any chance, are you working on something interesting [that you are allowed to talk about]? Am I wrong in assuming that reboot() shouldn't be used anywhere but in PID 1?

Nothing terribly interesting. I work in embedded systems and have seen cases where a poorly behaving process/init script can block the system from rebooting when going through init. Calling boot directly is something I might do in logic after a long delay to ensure that the system does reboot. There aren't any humans around to hold down the power button...