diff --git a/api_client/src/lib.rs b/api_client/src/lib.rs index ef9147fe9..52e85a336 100644 --- a/api_client/src/lib.rs +++ b/api_client/src/lib.rs @@ -21,8 +21,14 @@ pub enum Error { MissingProtocol, #[error("Error parsing HTTP Content-Length field")] ContentLengthParsing(#[source] std::num::ParseIntError), - #[error("Server responded with an error: {0:?}: {1:?}")] - ServerResponse(StatusCode, Option), + #[error("Server responded with error {0:?}: {1:?}")] + ServerResponse( + StatusCode, + // TODO: Move `api` module from `vmm` to dedicated crate and use a common type definition + Option< + String, /* Untyped: Currently Vec of error messages from top to root cause */ + >, + ), } #[derive(Clone, Copy, Debug)] diff --git a/src/bin/ch-remote.rs b/src/bin/ch-remote.rs index 4ef0b0257..9d8c4f68b 100644 --- a/src/bin/ch-remote.rs +++ b/src/bin/ch-remote.rs @@ -1132,7 +1132,70 @@ fn main() { }; if let Err(top_error) = target_api.do_command(&matches) { - cloud_hypervisor::cli_print_error_chain(&top_error, "ch-remote"); + // Helper to join strings with a newline. + fn join_strs(mut acc: String, next: String) -> String { + if !acc.is_empty() { + acc.push('\n'); + } + acc.push_str(&next); + acc + } + + // This function helps to modify the Display representation of remote + // API failures so that it aligns with the regular output of error + // messages. As we transfer a deep/rich chain of errors as String via + // the HTTP API, the nested error chain is lost. We retrieve it from + // the error response. + // + // In case the repose itself is broken, the error is printed directly + // by using the `X` level. + fn server_api_error_display_modifier( + level: usize, + indention: usize, + error: &(dyn std::error::Error + 'static), + ) -> Option { + if let Some(api_client::Error::ServerResponse(status_code, body)) = + error.downcast_ref::() + { + let body = body.as_ref().map(|body| body.as_str()).unwrap_or(""); + + // Retrieve the list of error messages back. + let lines: Vec<&str> = match serde_json::from_str(body) { + Ok(json) => json, + Err(e) => { + return Some(format!( + "{idention}X: Can't get remote's error messages from JSON response: {e}: body='{body}'", + idention = " ".repeat(indention) + )); + } + }; + + let error_status = format!("Server responded with {status_code:?}"); + // Prepend the error status line to the lines iter. + let lines = std::iter::once(error_status.as_str()).chain(lines); + let error_msg_multiline = lines + .enumerate() + .map(|(index, error_msg)| (index + level, error_msg)) + .map(|(level, error_msg)| { + format!( + "{idention}{level}: {error_msg}", + idention = " ".repeat(indention) + ) + }) + .fold(String::new(), join_strs); + + return Some(error_msg_multiline); + } + + None + } + + let top_error: &dyn std::error::Error = &top_error; + cloud_hypervisor::cli_print_error_chain( + top_error, + "ch-remote", + server_api_error_display_modifier, + ); process::exit(1) }; } diff --git a/src/lib.rs b/src/lib.rs index 94d5a7b48..1596a13f4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,7 +7,16 @@ use std::error::Error; /// Prints a chain of errors to the user in a consistent manner. /// The user will see a clear chain of errors, followed by debug output /// for opening issues. -pub fn cli_print_error_chain(top_error: &dyn Error, component: &str) { +pub fn cli_print_error_chain<'a>( + top_error: &'a (dyn Error + 'static), + component: &str, + // Function optionally returning the display representation of an error. + display_modifier: impl Fn( + /* level */ usize, + /*indention */ usize, + &'a (dyn Error + 'static), + ) -> Option, +) { eprint!("Error: {component} exited with the following "); if top_error.source().is_none() { eprintln!("error:"); @@ -21,7 +30,12 @@ pub fn cli_print_error_chain(top_error: &dyn Error, component: &str) { }) .enumerate() .for_each(|(level, error)| { - eprintln!(" {level}: {error}",); + // Special case: handling of HTTP Server responses in ch-remote + if let Some(message) = display_modifier(level, 2, error) { + eprintln!("{message}"); + } else { + eprintln!(" {level}: {error}"); + } }); } diff --git a/src/main.rs b/src/main.rs index 924cb19c3..4a0fbe91f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -884,7 +884,7 @@ fn main() { 0 } Err(top_error) => { - cloud_hypervisor::cli_print_error_chain(&top_error, "Cloud Hypervisor"); + cloud_hypervisor::cli_print_error_chain(&top_error, "Cloud Hypervisor", |_, _, _| None); 1 } }; diff --git a/vmm/src/api/http/mod.rs b/vmm/src/api/http/mod.rs index 9b2f5d1d8..69bc82dfe 100644 --- a/vmm/src/api/http/mod.rs +++ b/vmm/src/api/http/mod.rs @@ -4,6 +4,7 @@ // use std::collections::BTreeMap; +use std::error::Error; use std::fs::File; use std::os::unix::io::{IntoRawFd, RawFd}; use std::os::unix::net::UnixListener; @@ -69,20 +70,29 @@ pub enum HttpError { const HTTP_ROOT: &str = "/api/v1"; -/// Creates the error response's body meant to be sent back to an API client. +/// Creates the error response's JSON body meant to be sent back to an API client. +/// /// The error message contained in the response is supposed to be user-facing, /// thus insightful and helpful while balancing technical accuracy and /// simplicity. pub fn error_response(error: HttpError, status: StatusCode) -> Response { let mut response = Response::new(Version::Http11, status); - // We must use debug output here without `#`, as it is currently the only - // feasible option to get all relevant error details to the receiver, - // i.e., ch-remote, in a balanced form. The Display impl is not guaranteed - // to hold all relevant or helpful data. - // - // TODO: We might print a nice error chain here as well and send it to the - // remote, similar to the normal error reporting? - response.set_body(Body::new(format!("{error:?}"))); + + let error: &dyn Error = &error; + // Write the Display::display() output all errors (from top to root). + let error_messages = std::iter::successors(Some(error), |sub_error| { + // Dereference necessary to mitigate rustc compiler bug. + // See + (*sub_error).source() + }) + .map(|error| format!("{error}")) + .collect::>(); + + // TODO: Move `api` module from `vmm` to dedicated crate and use a common type definition + let json = serde_json::to_string(&error_messages).unwrap(); + + let body = Body::new(json); + response.set_body(body); response }