Feature/improve exec error message #71

AngeCyp · 2024-11-27T11:34:53Z

Fix #6

This PR add a better message when using the exec mode and if the executable is not found.

app-agent/src/exec_process.rs

TheElectronWill · 2024-11-28T09:03:47Z

app-agent/src/exec_process.rs

+        Err(e) => match e.kind() {
+            std::io::ErrorKind::NotFound => {
+                let directory_entries_iter = fs::read_dir("./").unwrap();
+                let mut entry_list = String::from("Available files in the current directory:\n");


I don't think it's up to the agent to replace ls.
How about just doing these two things:

If ErrorKind::NotFound

check if a file with the same name exists in the current directory

print:

command 'XYZ' not found Hint: A file named 'XYZ' exists in the current directory. Prepend ./ to execute it. Example: CMD exec './XYZ'

where CMD is the alumet binary (in the rapl plugin you can find an example of how to get the path to the current alumet binary on the user).

Bonus

If you want to have fun, implement a fuzzy finder to fix the potential typos :)
Example of what it would look like:

> ls script.sh > alumet-local-agent exec ./scrit.sh ERROR: ... Hint: did you mean './script.sh'?

Other example:

> ls script.sh > alumet-local-agent exec ./script ERROR: ... Hint: did you mean './script.sh'?

If permission denied

check if the file exists but does not have the X or R permission

print:

file 'XYZ' is missing the following permissions: PERM Hint: try chmod +PERM 'XYZ'

where PERM is r, x or rx

TheElectronWill

When done, it will be a nice improvement to the agent exec mode!

.cspell/project-words.txt

TheElectronWill · 2024-12-10T14:04:24Z

app-agent/src/exec_process.rs

+        Err(e) => match e.kind() {
+            std::io::ErrorKind::NotFound => {
+                let return_error: String = handle_not_found(external_command, args);
+                panic!("{}", return_error);
+            }
+            std::io::ErrorKind::PermissionDenied => {
+                let return_error: String = handle_permission_denied(external_command);
+                panic!("{}", return_error);
+            }
+            _ => {
+                panic!("Error in child process");
+            }


The function returns a Result, you should use that instead of panics.

As in the main you used expect() I expected to panic in case of error

TheElectronWill · 2024-12-10T14:04:54Z

app-agent/src/exec_process.rs

+    let file_permission_denied = match File::open(external_command.clone()) {
+        Ok(file) => file,
+        Err(err) => {
+            panic!("Error when try to read the file: {}", err);


TheElectronWill · 2024-12-10T14:06:15Z

app-agent/src/exec_process.rs

+        4 => "x",
+        _ => "",
+    };
+    if user_perm == "rx" || group_perm == "rx" || other_perm == "rx" {


What is all this doing, exactly? Isn't chmod +rx a solution that always work?

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

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?

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

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

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

app-agent/src/exec_process.rs

app-agent/src/utils.rs

…istance with adjacent transpositions

…ction and use it when file not found in exec mode

…ling on exec mode

…mmand to correct

TheElectronWill · 2025-01-08T13:41:01Z

app-agent/src/exec_process.rs

+                return Err(anyhow!(return_error));
+            }
+            _ => {
+                panic!("Error in child process");


exec_child returns a result, please remove the panics

TheElectronWill · 2025-01-08T13:42:58Z

app-agent/src/exec_process.rs

+    let file_permission_denied = match File::open(external_command.clone()) {
+        Ok(file) => file,
+        Err(err) => {


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.

TheElectronWill · 2025-01-08T13:43:31Z

app-agent/src/exec_process.rs

+            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");
+                            }
+                        }
+                    }
+                }
+            }


It would be cleaner to make it a separate function

TheElectronWill · 2025-01-08T13:44:11Z

app-agent/src/exec_process.rs

+    let user_perm = match file_metadata.permissions().mode() & 0o500 {
+        0 => "rx",
+        1 => "r",
+        4 => "x",
+        _ => "",
+    };


A function would be handy for deduplication here.

TheElectronWill · 2025-01-08T13:45:19Z

app-agent/src/exec_process.rs

+        4 => "x",
+        _ => "",
+    };
+    if user_perm == "rx" || group_perm == "rx" || other_perm == "rx" {


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

TheElectronWill · 2025-01-08T13:45:30Z

app-agent/src/exec_process.rs

+    } else {
+        log::warn!("Can't determine right issue about the file: {}", external_command);
+    }
+    "Issue happened about file's permission".to_string()


which issue? Use proper errors propagation

TheElectronWill · 2025-01-08T13:47:27Z

app-agent/src/exec_process.rs

+                    args.iter()
+                        .map(|arg| {
+                            if arg.contains(' ') {
+                                format!("\"{}\"", arg)
+                            } else {
+                                arg.to_string()
+                            }
+                        })
+                        .collect::<Vec<_>>()
+                        .join(" ")


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

TheElectronWill · 2025-01-08T13:48:09Z

app-agent/src/exec_process.rs

+            }
+        }
+        None => {
+            log::warn!("💡 Hint: No matching file exists in the current directory. Prepend ./ to execute it.");


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

TheElectronWill · 2025-01-08T13:48:14Z

app-agent/src/exec_process.rs

+            log::warn!("💡 Hint: No matching file exists in the current directory. Prepend ./ to execute it.");
+        }
+    }
+    "Issue happened because the file was not found".to_string()


which issue?

TheElectronWill · 2025-01-08T13:48:41Z

app-agent/src/lib.rs

@@ -6,6 +6,7 @@ pub mod agent_util;
 pub mod config_ops;
 pub mod exec_process;
 pub mod options;
+pub mod utils;


Please find a better name than utils. I suggest word_distance

TheElectronWill · 2025-01-08T13:50:06Z

app-agent/src/utils.rs

@@ -0,0 +1,170 @@
+use std::collections::HashMap;
+
+// Damerau-Levenshtein distance


Use triple slash to document function. Please add more details so that we understand what this is without googling.

AngeCyp requested a review from TheElectronWill November 27, 2024 11:34

TheElectronWill requested changes Nov 28, 2024

View reviewed changes

TheElectronWill added the A:app-agent area: runnable agent application label Nov 28, 2024

AngeCyp force-pushed the feature/improve-exec-error-message branch from 3e8c5f6 to eeb5269 Compare November 29, 2024 12:20

AngeCyp requested a review from TheElectronWill November 29, 2024 12:26

TheElectronWill requested changes Dec 10, 2024

View reviewed changes

AngeCyp force-pushed the feature/improve-exec-error-message branch from 833e56b to 3788bde Compare December 11, 2024 16:46

AngeCyp requested a review from TheElectronWill December 11, 2024 17:10

AngeCyp force-pushed the feature/improve-exec-error-message branch from 3788bde to 69a75cf Compare January 6, 2025 08:31

AngeCyp added 9 commits January 7, 2025 11:06

feature: When file not found for alumet exec => better explanation

461389c

feature: Modify app-agent

7153675

feat(exec_process): Improve the error message when using exec mode

14576ce

feat(exec_process): Better lint

59c3bba

feat(exec_process): Added a better error handling

08b06ec

feat(exec_process): first implementation of the Damerau–Levenshtein d…

78f8b35

…istance with adjacent transpositions

feat(exec_process): Add test for the Damerau-Levenshtein distance fun…

968ba25

…ction and use it when file not found in exec mode

feat(exec): improved Damerau-Levenshtein distance algo and error hand…

df9027b

…ling on exec mode

feat(exec): Check for parent rights if an issue happen and suggest co…

264ef6f

…mmand to correct

AngeCyp force-pushed the feature/improve-exec-error-message branch from 69a75cf to 264ef6f Compare January 7, 2025 10:06

TheElectronWill requested changes Jan 8, 2025

View reviewed changes

TheElectronWill reviewed Jan 8, 2025

View reviewed changes

TheElectronWill assigned AngeCyp Jan 8, 2025

feat(exec_process): work on PR review, separation of function,...

4fc1d61

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/improve exec error message #71

Feature/improve exec error message #71

AngeCyp commented Nov 27, 2024

TheElectronWill Nov 28, 2024

TheElectronWill left a comment

TheElectronWill Dec 10, 2024

AngeCyp Dec 11, 2024

TheElectronWill Dec 10, 2024

TheElectronWill Dec 10, 2024

AngeCyp Dec 11, 2024

TheElectronWill Dec 11, 2024

AngeCyp Dec 11, 2024

AngeCyp Dec 11, 2024 •

edited

Loading

TheElectronWill Jan 8, 2025

TheElectronWill Jan 8, 2025

TheElectronWill Jan 8, 2025

TheElectronWill Jan 8, 2025

TheElectronWill Jan 8, 2025

TheElectronWill Jan 8, 2025

TheElectronWill Jan 8, 2025

TheElectronWill Jan 8, 2025

TheElectronWill Jan 8, 2025

TheElectronWill Jan 8, 2025

TheElectronWill Jan 8, 2025

TheElectronWill Jan 8, 2025

		@@ -0,0 +1,170 @@
		use std::collections::HashMap;

		// Damerau-Levenshtein distance

Feature/improve exec error message #71

Are you sure you want to change the base?

Feature/improve exec error message #71

Conversation

AngeCyp commented Nov 27, 2024

Choose a reason for hiding this comment

If ErrorKind::NotFound

Bonus

If permission denied

TheElectronWill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AngeCyp Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

If `ErrorKind::NotFound`

AngeCyp Dec 11, 2024 •

edited

Loading