Improve error handling by using std lib rust code #4

Open
isa wants to merge 2 commits from isa/citadel-tools:error_handling into master
Contributor
No description provided.
isa added 1 commit 2024-08-29 14:56:59 -04:00
isa added 1 commit 2024-08-29 15:40:45 -04:00
brl reviewed 2024-08-30 11:29:03 -04:00
brl left a comment
Owner

Sorry, but no.

If you want to commit something this intrusive you need to start by explaining why the intrusive change is necessary. Also see the many specific comments I made.

Sorry, but no. If you want to commit something this intrusive you need to start by explaining why the intrusive change is necessary. Also see the many specific comments I made.
@ -14,3 +14,3 @@
const IMAGE_DIRECTORY: &str = "/run/citadel/images";
pub fn live_rootfs() -> Result<()> {
pub fn live_rootfs() -> Result<(), Box<dyn std::error::Error>> {
Owner

There's a common idiom for using result::Result<T,E> where you create a new type also called Result that specifies the error type you want to use.

for example:

type Result<T> = result::Result<T, Box<dyn std::error::Error>>;
There's a common idiom for using `result::Result<T,E>` where you create a new type also called `Result` that specifies the error type you want to use. for example: ```rust type Result<T> = result::Result<T, Box<dyn std::error::Error>>; ```
@ -61,3 +60,3 @@
}
fn kernel_version() -> String {
fn kernel_version() -> Result<String, Box<dyn std::error::Error>> {
Owner

What? Why does this need a Result return type?

What? Why does this need a `Result` return type?
@ -129,3 +129,3 @@
}
}
bail!("unable to find rootfs resource image in {}", IMAGE_DIRECTORY)
Result::Err(
Owner

This is an improvement over the current error handling API?

This is an improvement over the current error handling API?
@ -27,4 +26,1 @@
if let Err(ref e) = result {
warn!("Failed: {}", e);
exit(1);
Owner

Is rewriting this to not return a specific exit code the best plan? I don't know if this ever worked as intended to cause the corresponding systemd units to fail and then bring the system into a state where the problem can be understood and diagnosed, but now the utility has an undefined exit code that we may end up relying on.

Is rewriting this to not return a specific exit code the best plan? I don't know if this ever worked as intended to cause the corresponding systemd units to fail and then bring the system into a state where the problem can be understood and diagnosed, but now the utility has an undefined exit code that we may end up relying on.
@ -22,4 +21,4 @@
Some(s) if s == "setup" => do_setup(),
Some(s) if s == "boot-automount" => do_boot_automount(),
Some(s) if s == "start-realms" => do_start_realms(),
_ => Err(format_err!("Bad or missing argument").into()),
Owner

Ooops, you forgot to replace a libcitadel::error::Error the 'improved' version and everything still works when you return it. Are you still sure rewriting everything was what you needed to do?

Ooops, you forgot to replace a `libcitadel::error::Error` the 'improved' version and everything still works when you return it. Are you still sure rewriting everything was what you needed to do?
@ -37,3 +32,3 @@
live::live_rootfs()
} else {
rootfs::setup_rootfs()
Ok(rootfs::setup_rootfs()?)
Owner

No, don't do that. It misleads reader to think there is a return value from both of these functions.

Do this:

rootfs::setup_rootfs()?;
Ok(())

No, don't do that. It misleads reader to think there is a return value from both of these functions. Do this: ```rust rootfs::setup_rootfs()?; Ok(()) ```
@ -138,3 +147,2 @@
// Compare versions and channels
let a_v = a.metainfo().version();
let b_v = b.metainfo().version();
let meta_a = a.metainfo();
Owner

Why did you rewrite this? Why is this buried in a massive commit described vaguely as "improve error handling"?

Why did you rewrite this? Why is this buried in a massive commit described vaguely as "improve error handling"?
@ -162,3 +163,3 @@
let path = arg_matches.value_of("path").expect("path argument missing");
if !Path::new(path).exists() {
bail!("Cannot load image {}: File does not exist", path);
panic!("Cannot load image {}: File does not exist", path);
Owner

Uh..... you want to panic instead of just exiting cleanly and printing an error message?

Uh..... you want to panic instead of just exiting cleanly and printing an error message?
@ -509,2 +509,2 @@
("$TARGET", self.target_str())
])
pub fn finish_install(&self) -> Result<(), Box<dyn std::error::Error>> {
self.cmd_list(FINISH_COMMANDS, &[("$TARGET", self.target_str())])
Owner

When you mix reformatting changes in with code changes then I have to read the reformatting changes very carefully to make sure you haven't changed anything.

BTW, are you sure this is more readable than the way it was formatted previously?

When you mix reformatting changes in with code changes then I have to read the reformatting changes very carefully to make sure you haven't changed anything. BTW, are you sure this is more readable than the way it was formatted previously?
Owner

PS: Gitea is pretty sweet. I had no idea it would collect all the code review comments I wrote and show them like that.

PS: Gitea is pretty sweet. I had no idea it would collect all the code review comments I wrote and show them like that.
This pull request has changes conflicting with the target branch.
  • citadel-tool/src/image/mod.rs
  • citadel-tool/src/main.rs
  • citadel-tool/src/realmfs/mod.rs
  • citadel-tool/src/sync/desktop_sync.rs

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u error_handling:isa-error_handling
git checkout isa-error_handling
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: brl/citadel-tools#4
No description provided.