From 190a11f2124b0b60a2d44e85b7c9988373acfb6d Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Tue, 10 Jun 2025 10:23:41 +0200 Subject: [PATCH] ch-remote: also pretty-print remote server errors Remote server errors are transferred as raw HTTP body. This way, we lose the nested structured error information. This is an attempt to retrieve the errors from the HTTP response and to align the output with the normal error output. For example, this produces the following chain of errors. Note that everything after level 0 was retrieved from the HTTP server response: ``` Error: ch-remote exited with the following chain of errors: 0: http client error 1: Server responded with InternalServerError 2: Error from API 3: The disk could not be added to the VM 4: Failed to validate config 5: Identifier disk1 is not unique Debug Info: HttpApiClient(ServerResponse(InternalServerError, Some("Error from API
The disk could not be added to the VM
Failed to validate config
Identifier disk1 is not unique"))) ``` In case the JSON can't be parsed properly, ch-remote will print: ``` Error: ch-remote exited with the following chain of errors: 0: http client error X: Can't get remote's error messages from JSON response: EOF while parsing a value at line 1 column 0: body='' Debug Info: HttpApiClient(ServerResponse(InternalServerError, Some(""))) ``` Signed-off-by: Philipp Schuster On-behalf-of: SAP philipp.schuster@sap.com --- api_client/src/lib.rs | 10 +++++-- src/bin/ch-remote.rs | 65 ++++++++++++++++++++++++++++++++++++++++- src/lib.rs | 18 ++++++++++-- src/main.rs | 2 +- vmm/src/api/http/mod.rs | 28 ++++++++++++------ 5 files changed, 108 insertions(+), 15 deletions(-) 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 }