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

Jump to bottom

Improve handling of systemd activation #50

Merged
merged 2 commits into from
Oct 21, 2019
Merged

Improve handling of systemd activation #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b84a484
Clarify licenses of some dependencies
lnicola Oct 20, 2019
d07bf5e
Improve handling of systemd activation
lnicola Oct 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions Cargo.toml
View file
Open in desktop
Expand Up @@ -17,7 +17,7 @@ uuid = "0.7.4"
threadpool = "1.7.1"
std-semaphore = "0.1.0"
signal-hook = "0.1.10"
sd-notify = { version = "0.1.0", optional = true }
sd-notify = { version = "0.1.1" }
toml = "0.4.2"
serde = { version = "1.0", features = ["derive"] }
env_logger = "0.7.1"
Expand All @@ -42,4 +42,4 @@ mbed = []

# Feature to compile the PARSEC binary to be executed as a systemd daemon.
# This feature is only available on Linux.
systemd-daemon = ["sd-notify"]
systemd-daemon = []
10 changes: 5 additions & 5 deletions README.md
View file
Open in desktop
Expand Up @@ -128,19 +128,19 @@ You will need to understand the [**wire protocol specification**](docs/wire_prot
The software is provided under Apache-2.0. Contributions to this project are accepted under the same license.

This project uses the following third party crates:
* serde (Apache-2.0)
* serde (MIT and Apache-2.0)
Copy link
Member

Choose a reason for hiding this comment

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

When the crate choose a double-licensing option (Apache-2.0 OR MIT), as the following (example uuid):

License

Licensed under either of:

Contribution

Unless you explicitly state otherwise, any contribution intentionally submitted for inclusion in the work by you, as defined in the Apache-2.0 license, shall be dual licensed as above, without any additional terms or conditions.

We explicitely make a choice and choose in that file to use Apache-2.0 license. If it is AND, we keep the two licenses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can revert the licensing change, but that sounds like a false dichotomy. All of those crates are dual-licensed, in the sense that you can pick either of them. The difference you noticed is in the manifest. People used to write MIT/Apache-2.0, but now MIT OR Apache-2.0 is preferred because it's "more machine-readable". Both are equivalent, see the comment in https://doc.rust-lang.org/stable/cargo/reference/manifest.html#package-metadata.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the pointer. What I understand from that is that is is more explicit to use AND or OR instead of a slash (/) but not that they both mean the same thing 😛
But I do not know if the licenses are for use of the library or contributions to it, and if it is for contributions that means that there could be files both licensed under MIT and Apache-2.0 in the same library.
In that case, it is probably better to use AND everywhere as you changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think OR would be a better fit.

But I do not know if the licenses are for use of the library or contributions to it, and if it is for contributions that means that there could be files both licensed under MIT and Apache-2.0 in the same library.

It's for the crate, and dual-licensing means that the user of the crate gets to pick one of the licenses (or both) that fits their project better. By contributing to a crate you implicitly admit that your changes will be licensed under the crate's terms (e.g. both MIT and Apache-2.0).

Copy link
Member

Choose a reason for hiding this comment

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

Would not it feel weird to have OR on this file as we are building a binary and not re-distributing the crates? As in, we are the last one in the chain so should not we make that decision between Apache-2.0 or MIT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to say "PARSEC is licensed under Apache-2.0 and uses the following dependencies, with their associated licenses", but I am not a lawyer :-).

* bindgen (BSD-3-Clause)
* cargo\_toml (Apache-2.0)
* toml (MIT and Apache-2.0)
* rand (MIT and Apache-2.0)
* base64 (MIT and Apache-2.0)
* uuid (Apache-2.0)
* threadpool (Apache-2.0)
* uuid (MIT and Apache-2.0)
* threadpool (MIT and Apache-2.0)
* std-semaphore (MIT and Apache-2.0)
* num\_cpus (MIT and Apache-2.0)
* signal-hook (MIT and Apache-2.0)
* sd-notify (Apache-2.0)
* log (Apache-2.0)
* sd-notify (MIT and Apache-2.0)
* log (MIT and Apache-2.0)
* env\_logger (MIT and Apache-2.0)

This project uses the following third party libraries:
Expand Down
1 change: 0 additions & 1 deletion src/bin/main.rs
View file
Open in desktop
Expand Up @@ -57,7 +57,6 @@ fn main() -> Result<(), Error> {

info!("PARSEC is ready.");

#[cfg(feature = "systemd-daemon")]
// Notify systemd that the daemon is ready, the start command will block until this point.
let _ = sd_notify::notify(true, &[sd_notify::NotifyState::Ready]);

Expand Down
122 changes: 53 additions & 69 deletions src/front/domain_socket.rs
View file
Open in desktop
Expand Up @@ -31,7 +31,7 @@ static SOCKET_PATH: &str = "/tmp/security-daemon-socket";
///
/// Only works on Unix systems.
pub struct DomainSocketListener {
listener: Option<UnixListener>,
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to remove that to me! As the DomainSocketListener can only be instanciated by the builder, this will never be None.

listener: UnixListener,
timeout: Duration,
}

Expand All @@ -42,36 +42,39 @@ impl DomainSocketListener {
/// - if a file/socket exists at the path specified for the socket and `remove_file`
/// fails
/// - if binding to the socket path fails
fn init(&mut self) {
if cfg!(feature = "systemd-daemon") {
// The PARSEC service is socket activated (see parsec.socket file).
// systemd creates the PARSEC service giving it an initialised socket as the file
// descriptor number 3 (see sd_listen_fds(3) man page).
// If an instance of PARSEC compiled with the "systemd-daemon" feature is run directly
// instead of by systemd, this call will still work but the next accept call on the
// UnixListener will generate a Linux error 9 (Bad file number), as checked below.
unsafe {
self.listener = Some(UnixListener::from_raw_fd(3));
}
} else {
let socket = Path::new(SOCKET_PATH);
pub fn new(timeout: Duration) -> Self {
// If this PARSEC instance was socket activated (see the `parsec.socket`
// file), the listener will be opened by systemd and passed to the
// process.
// If PARSEC was service activated or not started under systemd, this
// will return `0`.
let listener =
match sd_notify::listen_fds().expect("Could not retrieve listener from systemd") {
0 => {
let socket = Path::new(SOCKET_PATH);

if socket.exists() {
fs::remove_file(&socket).unwrap();
}
if socket.exists() {
fs::remove_file(&socket).unwrap();
}

let listener_val = match UnixListener::bind(SOCKET_PATH) {
Ok(listener) => listener,
Err(err) => panic!(err),
};
let listener =
UnixListener::bind(SOCKET_PATH).expect("Could not bind listen socket");
listener
.set_nonblocking(true)
.expect("Could not set the socket as non-blocking");

// Set the socket as non-blocking.
listener_val
.set_nonblocking(true)
.expect("Could not set the socket as non-blocking");
lnicola marked this conversation as resolved.
Show resolved Hide resolved
listener
}
1 => {
// No need to set the socket as non-blocking, parsec.service
// already requests that.
let nfd = sd_notify::SD_LISTEN_FDS_START;
unsafe { UnixListener::from_raw_fd(nfd) }
}
_ => panic!("Received too many file descriptors"),
};

self.listener = Some(listener_val);
}
Self { listener, timeout }
}
}

Expand All @@ -81,44 +84,30 @@ impl Listen for DomainSocketListener {
}

fn accept(&self) -> Option<Box<dyn ReadWrite + Send>> {
if let Some(listener) = &self.listener {
let stream_result = listener.accept();
match stream_result {
Ok((stream, _)) => {
if let Err(err) = stream.set_read_timeout(Some(self.timeout)) {
error!("Failed to set read timeout ({})", err);
None
} else if let Err(err) = stream.set_write_timeout(Some(self.timeout)) {
error!("Failed to set write timeout ({})", err);
None
} else if let Err(err) = stream.set_nonblocking(false) {
error!("Failed to set stream as blocking ({})", err);
None
} else {
Some(Box::from(stream))
}
}
Err(err) => {
if cfg!(feature = "systemd-daemon") {
// When run as a systemd daemon, a file descriptor mapping to the Domain Socket
// should have been passed to this process.
if let Some(os_error) = err.raw_os_error() {
// On Linux, 9 is EBADF (Bad file number)
if os_error == 9 {
panic!("The Unix Domain Socket file descriptor (number 3) should have been given to this process.");
}
}
}
// Check if the error is because no connections are currently present.
if err.kind() != ErrorKind::WouldBlock {
// Only log the real errors.
error!("Failed to connect with a UnixStream ({})", err);
}
let stream_result = self.listener.accept();
match stream_result {
Ok((stream, _)) => {
if let Err(err) = stream.set_read_timeout(Some(self.timeout)) {
error!("Failed to set read timeout ({})", err);
None
} else if let Err(err) = stream.set_write_timeout(Some(self.timeout)) {
error!("Failed to set write timeout ({})", err);
None
} else if let Err(err) = stream.set_nonblocking(false) {
error!("Failed to set stream as blocking ({})", err);
None
} else {
Some(Box::from(stream))
}
}
Err(err) => {
// Check if the error is because no connections are currently present.
if err.kind() != ErrorKind::WouldBlock {
// Only log the real errors.
error!("Failed to connect with a UnixStream ({})", err);
}
None
}
} else {
panic!("The Unix Domain Socket has not been initialised.");
}
}
}
Expand All @@ -139,12 +128,7 @@ impl DomainSocketListenerBuilder {
}

pub fn build(self) -> DomainSocketListener {
let mut listener = DomainSocketListener {
timeout: self.timeout.expect("FrontEndHandler missing"),
listener: None,
};
listener.init();

listener
let timeout = self.timeout.expect("The listener timeout was not set");
DomainSocketListener::new(timeout)
}
}