Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add reboot() support on Linux #386

Merged
merged 10 commits into from
Jul 16, 2016
Merged

Add reboot() support on Linux #386

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f3d7b01
Add the initial implementation of reboot()
bugaevc Jul 11, 2016
91b29ab
Add some basic docs for reboot::set_cad_enabled()
bugaevc Jul 11, 2016
c63f89c
Ignore the overflow for constants
bugaevc Jul 11, 2016
bf796cc
Stop targeting Android
bugaevc Jul 13, 2016
b19afe5
Use libc's declarations
bugaevc Jul 13, 2016
012c662
Manually match on RebootMode::*
bugaevc Jul 13, 2016
cce6bce
Switch Clone and Copy
bugaevc Jul 13, 2016
f682458
Add some documentation
bugaevc Jul 13, 2016
8f6b162
Revert "Manually match on RebootMode::*"
bugaevc Jul 14, 2016
215f387
Name enum variants after the libc constants
bugaevc Jul 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/sys/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ pub mod stat;
#[cfg(any(target_os = "linux", target_os = "android"))]
pub mod syscall;

#[cfg(any(target_os = "linux"))]
pub mod reboot;

#[cfg(not(target_os = "ios"))]
pub mod termios;

Expand Down
44 changes: 44 additions & 0 deletions src/sys/reboot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
use {Errno, Error, Result};
use libc;
use void::Void;
use std::mem::drop;

#[derive(Copy, Clone, Debug, Eq, PartialEq)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@bugaevc bugaevc Jul 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pub enum RebootMode {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Halt,
kexec,
PowerOff,
Restart,
// we do not support Restart2,
Suspend,
}

pub fn reboot(how: RebootMode) -> Result<Void> {
let cmd = match how {
RebootMode::Halt => libc::RB_HALT_SYSTEM,
RebootMode::kexec => libc::RB_KEXEC,
RebootMode::PowerOff => libc::RB_POWER_OFF,
RebootMode::Restart => libc::RB_AUTOBOOT,
// we do not support Restart2,
RebootMode::Suspend => libc::RB_SW_SUSPEND,
};
unsafe {
libc::reboot(cmd)
};
Err(Error::Sys(Errno::last()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errno::result(res).map(drop) should work here just like in set_cad_enabled, so that we consistently have Result<()> as the return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the whole point is that reboot() never returns, unless there's an error. Same thing as execvp and friends.
set_cad_enabled() (btw is that a good name?) on the other hand returns if everything went fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what the manual says:

For the values of cmd that stop or restart the system, a successful call to reboot() does not return. For the other cmd values, zero is returned on success. In all cases, -1 is returned on failure, and errno is set appropriately.

}

/// Enable or disable the reboot keystroke (Ctrl-Alt-Delete).
///
/// Corresponds to calling `reboot(RB_ENABLE_CAD)` or `reboot(RB_DISABLE_CAD)` in C.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we mention in RebootMode's documentation, that this function exists, in case someone is looking for that functionality.

Copy link
Contributor Author

@bugaevc bugaevc Jul 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will do. Maybe I should just document everything.

pub fn set_cad_enabled(enable: bool) -> Result<()> {
let cmd = if enable {
libc::RB_ENABLE_CAD
} else {
libc::RB_DISABLE_CAD
};
let res = unsafe {
libc::reboot(cmd)
};
Errno::result(res).map(drop)
}