idiomatic error handling #1

Closed
ramrem wants to merge 3 commits from idiomatic_error_handling into master
First-time contributor

New error handling with thiserror

New error handling with thiserror
ramrem added 3 commits 2025-01-10 19:23:24 +00:00
ghe0 requested changes 2025-01-10 19:32:45 +00:00
@ -113,4 +141,0 @@
let decrypt_result = os::try_backup_keyfile(&form.keyfile);
if let Err(decryption_error) = decrypt_result {
return HttpResponse::BadRequest()
.body(format!("Could not decrypt root: {decryption_error:?}"));
Owner

Did we a test with a corrupted disk to see what error is displayed for the CLI?

Did we a test with a corrupted disk to see what error is displayed for the CLI?
@ -36,3 +68,2 @@
fn get_cert_hash() -> [u8; 64] {
fn get_cert_hash() -> Result<[u8; 64], DtrfsError> {
let mut hasher = Sha3_512::new();
let crt = File::open(CRT_FILE).expect("Could not open crt file.");
Owner

I see this error message got removed. In case dtrfs fails now, what error will be displayed in the console of the VM?
If someone runs this code to troubleshoot potential issues or to expand functionality, will that person receive an error informing about the missing certificate?

I see this error message got removed. In case dtrfs fails now, what error will be displayed in the console of the VM? If someone runs this code to troubleshoot potential issues or to expand functionality, will that person receive an error informing about the missing certificate?
@ -18,0 +42,4 @@
impl ResponseError for DtrfsError {
fn error_response(&self) -> HttpResponse {
match self {
error => HttpResponse::InternalServerError().body(format!("{}", error)),
Owner

Is it possible to respect HTTP error codes? Link here: https://en.wikipedia.org/wiki/List_of_HTTP_status_codes

Maybe a match on DtrfsError?

Is it possible to respect HTTP error codes? Link here: https://en.wikipedia.org/wiki/List_of_HTTP_status_codes Maybe a match on DtrfsError?
@ -100,0 +125,4 @@
req: HttpRequest,
form: web::Form<InstallForm>,
) -> Result<HttpResponse, Error> {
verify(&req)?;// TODO: This is temporary, we need to merget from the other branch
Owner

Not sure what is temporary.

Not sure what is temporary.
@ -4,3 +4,2 @@
use sev::firmware::guest::{AttestationReport, DerivedKey, Firmware, GuestFieldSelect};
fn request_hardware_report(data: [u8; 64]) -> Result<AttestationReport> {
let mut fw = Firmware::open().context("unable to open /dev/sev-guest")?;
Owner

I don't see mention of /dev/sev-guest anywhere. I believe it is important to mention this file if the firmware fails to open.

I don't see mention of `/dev/sev-guest` anywhere. I believe it is important to mention this file if the firmware fails to open.
Owner

After going through this code, we figured out dtrfs has particular error requirements, because:

  1. the errors must be designed for the CLI
  2. dtrfs has no logs, meaning point 1 is a lot stronger that it seems

As these requirements were not clear before this branch got started, we will consider this a great exercise.

@ramrem Congratulations on learning how to write idiomatic errors in Rust! We appreciate it! No hard feelings. We will redesign this.

All Love. Closed.

After going through this code, we figured out dtrfs has particular error requirements, because: 1. the errors must be designed for the CLI 2. dtrfs has no logs, meaning point 1 is a lot stronger that it seems As these requirements were not clear before this branch got started, we will consider this a great exercise. @ramrem Congratulations on learning how to write idiomatic errors in Rust! We appreciate it! No hard feelings. We will redesign this. All Love. Closed.
ghe0 closed this pull request 2025-01-11 15:59:00 +00:00

Pull request closed

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: testnet/dtrfs#1
No description provided.