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.

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 Rust toolchains and example rust library to Collector #1959

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Stringy
Copy link
Collaborator

@Stringy Stringy commented Nov 13, 2024

Description

Adds a Rust toolchain into Collector, allowing us to write Rust crates and call into them from C++. The idea is that we can gradually increase the amount of Rust in the project and take advantage of its helpful features including memory / thread safety.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a slightly modified version of the cmake module taken from here: https://github.com/micahsnyder/cmake-rust-demo/blob/main/cmake/FindRust.cmake

The main addition is an include directory to pass to the build commands so we can centralise cbindgen headers

@Stringy Stringy added the run-multiarch-builds Run steps for non-x86 archs. label Nov 14, 2024
collector/lib/rust/host/src/info.rs Outdated Show resolved Hide resolved
}

fn filter_by_key<T: Into<PathBuf>>(&self, path: T, name: &str) -> String {
if let Ok(file) = File::open(path.into()) {
Copy link
Collaborator

@Molter73 Molter73 Nov 14, 2024

Choose a reason for hiding this comment

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

You can probably remove one level of indentation with a let-else here

Suggested change
if let Ok(file) = File::open(path.into()) {
let Ok(file) = File::open(path.into()) else { return Default::default(); };

collector/lib/rust/host/src/kernel.rs Outdated Show resolved Hide resolved
collector/lib/rust/host/src/kernel.rs Outdated Show resolved Hide resolved
Comment on lines +213 to +260
let root = PathBuf::from(std::env::var("COLLECTOR_HOST_PATH").unwrap_or("/host".to_string()));
root.join(path.into())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let root = PathBuf::from(std::env::var("COLLECTOR_HOST_PATH").unwrap_or("/host".to_string()));
root.join(path.into())
let root = std::env::var("COLLECTOR_HOST_PATH").unwrap_or("/host".to_string());
PathBuf::from(root).join(path.into())

let mut distro = String::new();
let mut hostname = String::new();

if let Ok(file) = File::open("/etc/os-release") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to host_path this as well?

Suggested change
if let Ok(file) = File::open("/etc/os-release") {
if let Ok(file) = File::open(host_path("/etc/os-release")) {


for line in reader.lines() {
match line {
Ok(s) if s.starts_with("ID") => os = s["ID ".len()..].to_owned(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now I'm wondering, do we need to add the = to these?

Suggested change
Ok(s) if s.starts_with("ID") => os = s["ID ".len()..].to_owned(),
Ok(s) if s.starts_with("ID=") => os = s["ID=".len()..].to_owned(),

Comment on lines +59 to +73
let hostname_paths = vec![
host_path("/etc/hostname"),
host_path("/proc/sys/kernel/hostname"),
];
for path in hostname_paths {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a vector? I would assume you can just use an array here

Suggested change
let hostname_paths = vec![
host_path("/etc/hostname"),
host_path("/proc/sys/kernel/hostname"),
];
for path in hostname_paths {
for path in [
host_path("/etc/hostname"),
host_path("/proc/sys/kernel/hostname"),
] {


/// Whether or not this host machine is CORE-OS
pub fn is_coreos(&self) -> bool {
self.os_id().eq("coreos")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we use standard == in these checks?

Suggested change
self.os_id().eq("coreos")
self.os_id() == "coreos"

Comment on lines 152 to 191
let locations = vec![
// try canonical vmlinux BTF through sysfs first
("/sys/kernel/btf/vmlinux".to_string(), false),
// fall back to trying to find vmlinux on disk otherwise
(format!("/boot/vmlinux-{}", self.kernel.release), false),
(
format!(
"/lib/modules/{}/vmlinux-{}",
self.kernel.release, self.kernel.release
),
false,
),
(
format!("/lib/modules/{}/build/vmlinux", self.kernel.release),
false,
),
(
format!("/usr/lib/modules/{}/kernel/vmlinux", self.kernel.release),
true,
),
(
format!("/usr/lib/debug/boot/vmlinux-{}", self.kernel.release),
true,
),
(
format!("/usr/lib/debug/boot/vmlinux-{}.debug", self.kernel.release),
true,
),
(
format!("/usr/lib/debug/lib/modules/{}/vmlinux", self.kernel.release),
true,
),
];
Copy link
Collaborator

@Molter73 Molter73 Nov 15, 2024

Choose a reason for hiding this comment

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

Suggested change
let locations = vec![
// try canonical vmlinux BTF through sysfs first
("/sys/kernel/btf/vmlinux".to_string(), false),
// fall back to trying to find vmlinux on disk otherwise
(format!("/boot/vmlinux-{}", self.kernel.release), false),
(
format!(
"/lib/modules/{}/vmlinux-{}",
self.kernel.release, self.kernel.release
),
false,
),
(
format!("/lib/modules/{}/build/vmlinux", self.kernel.release),
false,
),
(
format!("/usr/lib/modules/{}/kernel/vmlinux", self.kernel.release),
true,
),
(
format!("/usr/lib/debug/boot/vmlinux-{}", self.kernel.release),
true,
),
(
format!("/usr/lib/debug/boot/vmlinux-{}.debug", self.kernel.release),
true,
),
(
format!("/usr/lib/debug/lib/modules/{}/vmlinux", self.kernel.release),
true,
),
];
let release = &self.kernel.release;
let locations = vec![
// try canonical vmlinux BTF through sysfs first
("/sys/kernel/btf/vmlinux".to_string(), false),
// fall back to trying to find vmlinux on disk otherwise
(format!("/boot/vmlinux-{release}"), false),
(format!("/lib/modules/{release}/vmlinux-{release}"), false),
(format!("/lib/modules/{release}/build/vmlinux"), false),
(format!("/usr/lib/modules/{release}/kernel/vmlinux"), true),
(format!("/usr/lib/debug/boot/vmlinux-{release}"), true,),
(format!("/usr/lib/debug/boot/vmlinux-{release}.debug"), true),
(format!("/usr/lib/debug/lib/modules/{release}/vmlinux"), true),
];

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could also directly call host_path on the ones that need to be mapped and drop from a vector of tuples to a vector of paths.

Comment on lines 186 to 198
for location in locations {
let path = if location.1 {
host_path(location.0)
} else {
PathBuf::from(location.0)
};

if path.exists() {
return true;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for location in locations {
let path = if location.1 {
host_path(location.0)
} else {
PathBuf::from(location.0)
};
if path.exists() {
return true;
}
}
for (path, on_host) in locations {
let path = if on_host {
host_path(path)
} else {
PathBuf::from(path)
};
if path.exists() {
return true;
}
}

}

impl KernelVersion {
pub fn new(release: &str, version: &str, machine: &str) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need these to be &str? Can't we use straight up String? Most places it looks like we already have those, it would save creating/copying strings.

@Stringy Stringy force-pushed the giles/rust-toolchain branch from 4ec4d61 to 6c88ace Compare December 23, 2024 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants