Add reboot() support on Linux #386

bugaevc · 2016-07-11T17:58:53Z

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 · 2016-07-11T18:05:22Z

To test, as root:

extern crate nix;

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

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

fiveop · 2016-07-12T07:05:20Z

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 · 2016-07-12T07:27:51Z

@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 · 2016-07-12T07:45:55Z

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 · 2016-07-12T10:47:21Z

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

bugaevc · 2016-07-12T22:08:05Z

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

posborne · 2016-07-13T00:22:13Z

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

Force using the constants even on x86 where they do not fit into isize (c_int)

bugaevc · 2016-07-13T07:40:47Z

Great! Should I merge or rebase?

bugaevc · 2016-07-13T17:48:09Z

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

bugaevc · 2016-07-13T17:57:32Z

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 · 2016-07-13T18:17:10Z

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

fiveop · 2016-07-13T19:15:35Z

src/sys/reboot.rs

+use void::Void;
+use std::mem::drop;
+
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]


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

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

fiveop · 2016-07-13T19:31:05Z

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 · 2016-07-13T22:09:31Z

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

fiveop · 2016-07-14T09:00:46Z

src/sys/reboot.rs

+/// See [`set_cad_enabled()`](fn.set_cad_enabled.html) for
+/// enabling/disabling Ctrl-Alt-Delete.
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub enum RebootMode {


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

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)

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.

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.

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

This reverts commit 012c662.

bugaevc · 2016-07-14T12:36:35Z

It's failing again...

fiveop · 2016-07-14T20:10:06Z

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 · 2016-07-14T20:54:40Z

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 · 2016-07-15T17:13:26Z

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

bugaevc · 2016-07-15T19:17:05Z

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 · 2016-07-15T21:07:54Z

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 · 2016-07-15T21:35:11Z

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 · 2016-07-15T23:02:54Z

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 · 2016-07-16T00:23:51Z

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 · 2016-07-16T06:36:22Z

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 · 2016-07-16T06:55:05Z

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 · 2016-07-16T12:27:28Z

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 · 2016-07-16T12:57:43Z

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 · 2016-07-16T12:58:52Z

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

fiveop · 2016-07-16T12:59:27Z

@homu r+

homu · 2016-07-16T12:59:53Z

📌 Commit 215f387 has been approved by fiveop

homu · 2016-07-16T13:00:13Z

⚡ Test exempted - status

Add reboot() support on Linux This adds support for calling [reboot(2)](http://linux.die.net/man/2/reboot) 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](http://www.freebsd.org/cgi/man.cgi?reboot(2))). 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.

fiveop · 2016-07-16T13:00:29Z

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

bugaevc · 2016-07-16T13:07:18Z

Great! Thank you for helping me get it done.

posborne · 2016-07-16T14:20:55Z

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

bugaevc mentioned this pull request Jul 12, 2016

Linux: add reboot() and constants rust-lang/libc#333

Merged

bugaevc added 5 commits July 13, 2016 10:19

Add the initial implementation of reboot()

f3d7b01

Add some basic docs for reboot::set_cad_enabled()

91b29ab

Ignore the overflow for constants

c63f89c

Force using the constants even on x86 where they do not fit into isize (c_int)

Stop targeting Android

bf796cc

Use libc's declarations

b19afe5

bugaevc force-pushed the reboot branch from 4aa96c0 to b19afe5 Compare July 13, 2016 17:49

Manually match on RebootMode::*

012c662

fiveop reviewed Jul 13, 2016
View reviewed changes

bugaevc added 2 commits July 14, 2016 00:40

Switch Clone and Copy

cce6bce

Add some documentation

f682458

fiveop reviewed Jul 14, 2016
View reviewed changes

bugaevc added 2 commits July 14, 2016 14:54

Revert "Manually match on RebootMode::*"

8f6b162

This reverts commit 012c662.

Name enum variants after the libc constants

215f387

fiveop mentioned this pull request Jul 15, 2016

Cast unsigned literals to signed integers. rust-lang/libc#334

Merged

homu merged commit 215f387 into nix-rust:master Jul 16, 2016

bugaevc deleted the reboot branch July 18, 2016 16:26

Add reboot() support on Linux #386

Add reboot() support on Linux #386

Conversation

bugaevc commented Jul 11, 2016 • edited Loading

bugaevc commented Jul 11, 2016

fiveop commented Jul 12, 2016 • edited Loading

bugaevc commented Jul 12, 2016

fiveop commented Jul 12, 2016

bugaevc commented Jul 12, 2016

bugaevc commented Jul 12, 2016

posborne commented Jul 13, 2016

bugaevc commented Jul 13, 2016

bugaevc commented Jul 13, 2016

bugaevc commented Jul 13, 2016

bugaevc commented Jul 13, 2016

fiveop Jul 13, 2016

Choose a reason for hiding this comment

bugaevc Jul 13, 2016 • edited Loading

Choose a reason for hiding this comment

fiveop commented Jul 13, 2016

bugaevc commented Jul 13, 2016

fiveop Jul 14, 2016

Choose a reason for hiding this comment

fiveop Jul 14, 2016

Choose a reason for hiding this comment

bugaevc Jul 14, 2016

Choose a reason for hiding this comment

fiveop Jul 14, 2016

Choose a reason for hiding this comment

bugaevc Jul 14, 2016

Choose a reason for hiding this comment

bugaevc commented Jul 14, 2016

fiveop commented Jul 14, 2016 via email

bugaevc commented Jul 14, 2016

posborne commented Jul 15, 2016

bugaevc commented Jul 15, 2016

fiveop commented Jul 15, 2016

bugaevc commented Jul 15, 2016

posborne commented Jul 15, 2016 • edited Loading

alexcrichton commented Jul 16, 2016

bugaevc commented Jul 16, 2016

bugaevc commented Jul 16, 2016

fiveop commented Jul 16, 2016 • edited Loading

bugaevc commented Jul 16, 2016

fiveop commented Jul 16, 2016

fiveop commented Jul 16, 2016 • edited Loading

homu commented Jul 16, 2016

homu commented Jul 16, 2016

fiveop commented Jul 16, 2016

bugaevc commented Jul 16, 2016

posborne commented Jul 16, 2016

bugaevc commented Jul 11, 2016 •

edited

Loading

fiveop commented Jul 12, 2016 •

edited

Loading

bugaevc Jul 13, 2016 •

edited

Loading

posborne commented Jul 15, 2016 •

edited

Loading

fiveop commented Jul 16, 2016 •

edited

Loading

fiveop commented Jul 16, 2016 •

edited

Loading