Improve error handling by using std lib rust code #4
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "isa/citadel-tools:error_handling"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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>> {
There's a common idiom for using
result::Result<T,E>
where you create a new type also calledResult
that specifies the error type you want to use.for example:
@ -61,3 +60,3 @@
}
fn kernel_version() -> String {
fn kernel_version() -> Result<String, Box<dyn std::error::Error>> {
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(
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);
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()),
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()?)
No, don't do that. It misleads reader to think there is a return value from both of these functions.
Do this:
@ -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();
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);
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())])
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?
PS: Gitea is pretty sweet. I had no idea it would collect all the code review comments I wrote and show them like that.
Checkout
From your project repository, check out a new branch and test the changes.