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

Feature/improve exec error message #71

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
8 changes: 8 additions & 0 deletions .cspell/project-words.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ alumet
ASCIINEMA
astring
Atos
capaldi
TheElectronWill marked this conversation as resolved.
Show resolved Hide resolved
cbindgen
cdylib
Cgroups
Expand All @@ -21,7 +22,10 @@ ITLB
Mispredicted
monitorable
mpoint
necron
Neron
NVML
olle
PERFMON
POWERCAP
psys
Expand All @@ -39,5 +43,9 @@ SYSFS
thiserror
timerfd
Trystram
tuut
Tweety
UCUM
uncore
круто
Привет
225 changes: 219 additions & 6 deletions app-agent/src/exec_process.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
//! Tie Alumet to a running process.
use std::process::ExitStatus;
use std::{
fs::{self, File, Metadata},
os::unix::{fs::PermissionsExt, process::ExitStatusExt},
path::PathBuf,
process::ExitStatus,
};

use alumet::{
pipeline::{
Expand All @@ -12,15 +17,31 @@ use alumet::{
plugin::event::StartConsumerMeasurement,
resources::ResourceConsumer,
};
use anyhow::Context;
use anyhow::{anyhow, Context};

/// Spawns a child process and waits for it to exit.
pub fn exec_child(external_command: String, args: Vec<String>) -> anyhow::Result<ExitStatus> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please include the error e in the returned errors, so that we don't lose the original information.

// Spawn the process.
let mut p = std::process::Command::new(external_command.clone())
.args(args)
.spawn()
.expect("error in child process");
let p_result = std::process::Command::new(external_command.clone())
.args(args.clone())
.spawn();

let mut p = match p_result {
TheElectronWill marked this conversation as resolved.
Show resolved Hide resolved
Ok(val) => val,
Err(e) => match e.kind() {
std::io::ErrorKind::NotFound => {
let return_error: String = handle_not_found(external_command, args);
return Err(anyhow!(return_error));
}
std::io::ErrorKind::PermissionDenied => {
let return_error: String = handle_permission_denied(external_command);
return Err(anyhow!(return_error));
}
_ => {
panic!("Error in child process");
Copy link
Contributor

Choose a reason for hiding this comment

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

exec_child returns a result, please remove the panics

}
},
};

// Notify the plugins that there is a process to observe.
let pid = p.id();
Expand All @@ -33,6 +54,198 @@ pub fn exec_child(external_command: String, args: Vec<String>) -> anyhow::Result
Ok(status)
}

fn handle_permission_denied(external_command: String) -> String {
let file_permission_denied = match File::open(external_command.clone()) {
Ok(file) => file,
Err(err) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

the Ok branch is very small, you can turn this into a if let instead. Also, what does file_permission_denied represent?

It would be cleaner to make it a separate function.

// Current parent can change if a parent of the parent don't have the correct rights
let mut current_parent = match std::path::Path::new(&external_command).parent() {
Some(parent) => parent,
None => return "".to_string(),
};
// Trough this loop I will iterate over parent of parent until I can retrieve metadata, it will show the first folder
// that I can't execute and suggest to the user to grant execution rights.
let metadata: Metadata;
loop {
match current_parent.metadata() {
Ok(metadata_parent) => {
metadata = metadata_parent;
break;
}
Err(_) => {
current_parent = match current_parent.parent() {
Some(parent) => parent,
None => {
panic!("Unable to retrieve a parent for your file");
}
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be cleaner to make it a separate function

let user_perm_parent = match metadata.permissions().mode() & 0o500 {
0o100 => 1,
_ => 0,
};
let group_perm_parent = match metadata.permissions().mode() & 0o050 {
0o010 => 1,
_ => 0,
};
let other_perm_parent = match metadata.permissions().mode() & 0o005 {
0o001 => 1,
_ => 0,
};
// Print warn message when parent folder's file has a missing execute rights
if user_perm_parent == 0 {
log::warn!(
"folder '{}' is missing the following permissions for user owner: 'x'",
current_parent.display()
)
}
if group_perm_parent == 0 {
log::warn!(
"folder '{}' is missing the following permissions for group owner: 'x'",
current_parent.display()
)
}
if other_perm_parent == 0 {
log::warn!(
"folder '{}' is missing the following permissions for other: 'x'",
current_parent.display()
)
}
if user_perm_parent == 0 || group_perm_parent == 0 || other_perm_parent == 0 {
log::info!("💡 Hint: try 'chmod +x {}'", current_parent.display())
}
panic!("Error when trying to read the file: {}", err);
}
};

// Get file metadata
let file_metadata = file_permission_denied
.metadata()
.expect(format!("Unable to retrieve metadata for: {}", external_command).as_str());
// Check for user permissions.
let user_perm = match file_metadata.permissions().mode() & 0o500 {
0 => "rx",
1 => "r",
4 => "x",
_ => "",
};
Copy link
Contributor

Choose a reason for hiding this comment

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

A function would be handy for deduplication here.

// Check for group permissions.
let group_perm: &str = match file_metadata.permissions().mode() & 0o050 {
0 => "rx",
1 => "r",
4 => "x",
_ => "",
};
// Check for other permissions.
let other_perm = match file_metadata.permissions().mode() & 0o005 {
0 => "rx",
1 => "r",
4 => "x",
_ => "",
};
if user_perm == "rx" || group_perm == "rx" || other_perm == "rx" {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is all this doing, exactly? Isn't chmod +rx a solution that always 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.

I check which rights are missing, the goal is to suggest only to add missing rights, if read is missing but not execute, i will suggest chmod +r only

Copy link
Contributor

Choose a reason for hiding this comment

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

That's very generous of your part, but chmod does the checking, no? If chmod +r works, chmod +rx will work too. Something that would really be helpful is, maybe, to check the permissions of the folder: what happens if the user wants to execute /a/b/exec but /a/b is not readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, if +r is missing, +rx will work too. I found it was more interesting for people to see that, for example, if only read right is missing, but the execute one is already present... But it's as you want, I can just suggest one chmod: chmod +rx.
About the parent folder, you're totally right, I will check this right in my next commit. I will only check the parent folder but not the parent of the parents, as it could be veryyyyyy long and not necessary, in my opinion

Copy link
Contributor Author

@AngeCyp AngeCyp Dec 11, 2024

Choose a reason for hiding this comment

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

Regarding to 3788bde I check parent of parent to be sure all folder in the path have at least the 'execute' right

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to do that, do it properly: compute the missing permissions and print the message that suggests it in one go, without repeating all the possibilities (lines 148 to 162 should be just 2 lines).

log::error!(
"file '{}' is missing the following permissions: 'rx'",
external_command
);
log::info!("💡 Hint: try 'chmod +rx {}'", external_command)
} else if user_perm == "r" || group_perm == "r" || other_perm == "r" {
log::error!("file '{}' is missing the following permissions: 'r'", external_command);
log::info!("💡 Hint: try 'chmod +r {}'", external_command)
} else if user_perm == "x" || group_perm == "x" || other_perm == "x" {
log::error!("file '{}' is missing the following permissions: 'x'", external_command);
log::info!("💡 Hint: try 'chmod +x {}'", external_command)
} else {
log::warn!("Can't determine right issue about the file: {}", external_command);
}
"Issue happened about file's permission".to_string()
Copy link
Contributor

Choose a reason for hiding this comment

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

which issue? Use proper errors propagation

}

fn handle_not_found(external_command: String, args: Vec<String>) -> String {
fn resolve_application_path() -> std::io::Result<PathBuf> {
std::env::current_exe()?.canonicalize()
}
log::error!("Command '{}' not found", external_command);
let directory_entries_iter = match fs::read_dir(".") {
Ok(directory) => directory,
Err(err) => {
panic!("Error when trying to read current directory: {}", err);
}
};
let app_path = resolve_application_path()
.ok()
.and_then(|p| p.to_str().map(|s| s.to_owned()))
.unwrap_or(String::from("path/to/agent"));

let mut lowest_distance = usize::MAX;
let mut best_element = None;

for entry_result in directory_entries_iter {
let entry = entry_result.unwrap();
let entry_type = entry.file_type().unwrap();
if entry_type.is_file() {
let entry_string = entry.file_name().into_string().unwrap();
let distance = super::utils::distance_with_adjacent_transposition(
external_command
.strip_prefix("./")
.unwrap_or(&external_command)
.to_string(),
entry_string.clone(),
);
if distance < 3 && distance < lowest_distance {
lowest_distance = distance;
best_element = Some((entry_string, distance));
}
}
}
match best_element {
Some((element, distance)) => {
if distance == 0 {
log::info!(
"💡 Hint: A file named '{}' exists in the current directory. Prepend ./ to execute it.",
element
);
log::info!(
"Example: {} exec ./{} {}",
app_path,
element,
args.iter()
.map(|arg| {
if arg.contains(' ') {
format!("\"{}\"", arg)
} else {
arg.to_string()
}
})
.collect::<Vec<_>>()
.join(" ")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please separate the construction of the list from the log call (same everywhere).

);
} else {
log::info!(
"💡 Hint: Did you mean ./{} {}",
element,
args.iter()
.map(|arg| {
if arg.contains(' ') {
format!("\"{}\"", arg)
} else {
arg.to_string()
}
})
.collect::<Vec<_>>()
.join(" ")
);
}
}
None => {
log::warn!("💡 Hint: No matching file exists in the current directory. Prepend ./ to execute it.");
Copy link
Contributor

Choose a reason for hiding this comment

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

What? If there's no matching file, prepending ./ will never work.

}
}
"Issue happened because the file was not found".to_string()
Copy link
Contributor

Choose a reason for hiding this comment

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

which issue?

}

// Triggers one measurement (on all sources that support manual trigger).
pub fn trigger_measurement_now(pipeline: &MeasurementPipeline) -> anyhow::Result<()> {
let control_handle = pipeline.control_handle();
Expand Down
1 change: 1 addition & 0 deletions app-agent/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ pub mod agent_util;
pub mod config_ops;
pub mod exec_process;
pub mod options;
pub mod utils;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please find a better name than utils. I suggest word_distance


/// Returns the path to the Alumet agent that is being executed.
pub fn resolve_application_path() -> std::io::Result<PathBuf> {
Expand Down
Loading