* [pbs-devel] [PATCH proxmox/proxmox-backup 0/1] Improve errors when panics occur and the panic message is a formatted (not static) string
@ 2025-01-21 9:36 Laurențiu Leahu-Vlăducu
2025-01-21 9:36 ` [pbs-devel] [PATCH proxmox-backup 1/1] backup-proxy and parallel_handler: Improved errors when panics occur and the panic message is a formatted (not static) string. This worked already for &str literals, but not for Strings Laurențiu Leahu-Vlăducu
2025-01-21 9:36 ` [pbs-devel] [PATCH proxmox 1/1] rest-server: " Laurențiu Leahu-Vlăducu
0 siblings, 2 replies; 6+ messages in thread
From: Laurențiu Leahu-Vlăducu @ 2025-01-21 9:36 UTC (permalink / raw)
To: pbs-devel; +Cc: Laurențiu Leahu-Vlăducu
While I was working on some patches, I noticed that error messages
were not properly logged in some situations where we were formatting
errors (thus not logging errors defined at compile-time).
This patch series fixes the issues I had by patching the proxmox-rest-server,
but also fixes similar issues in PBS itself from happening in the future.
proxmox-backup:
Laurențiu Leahu-Vlăducu (1):
backup-proxy and parallel_handler: Improved errors when panics occur
and the panic message is a formatted (not static) string. This
worked already for &str literals, but not for Strings.
src/bin/proxmox-backup-proxy.rs | 5 ++++-
src/tools/parallel_handler.rs | 8 +++++++-
2 files changed, 11 insertions(+), 2 deletions(-)
proxmox:
Laurențiu Leahu-Vlăducu (1):
rest-server: Improved errors when panics occur and the panic message
is a formatted (not static) string. This worked already for &str
literals, but not for Strings.
proxmox-rest-server/src/worker_task.rs | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* [pbs-devel] [PATCH proxmox-backup 1/1] backup-proxy and parallel_handler: Improved errors when panics occur and the panic message is a formatted (not static) string. This worked already for &str literals, but not for Strings. 2025-01-21 9:36 [pbs-devel] [PATCH proxmox/proxmox-backup 0/1] Improve errors when panics occur and the panic message is a formatted (not static) string Laurențiu Leahu-Vlăducu @ 2025-01-21 9:36 ` Laurențiu Leahu-Vlăducu 2025-01-21 10:56 ` Gabriel Goller 2025-01-21 9:36 ` [pbs-devel] [PATCH proxmox 1/1] rest-server: " Laurențiu Leahu-Vlăducu 1 sibling, 1 reply; 6+ messages in thread From: Laurențiu Leahu-Vlăducu @ 2025-01-21 9:36 UTC (permalink / raw) To: pbs-devel; +Cc: Laurențiu Leahu-Vlăducu Downcasting to both &str and String is also done by the Rust Standard Library in the default panic handler. See: https://github.com/rust-lang/rust/blob/b605c65b6eb5fa71783f8e26df69975f9f1680ee/library/std/src/panicking.rs#L777 Signed-off-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com> --- src/bin/proxmox-backup-proxy.rs | 5 ++++- src/tools/parallel_handler.rs | 8 +++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs index ce1be1c0..a7dab4ac 100644 --- a/src/bin/proxmox-backup-proxy.rs +++ b/src/bin/proxmox-backup-proxy.rs @@ -424,7 +424,10 @@ async fn run_task_scheduler() { match schedule_tasks().catch_unwind().await { Err(panic) => match panic.downcast::<&str>() { Ok(msg) => eprintln!("task scheduler panic: {msg}"), - Err(_) => eprintln!("task scheduler panic - unknown type"), + Err(panic) => match panic.downcast::<String>() { + Ok(msg) => eprintln!("task scheduler panic: {msg}"), + Err(_) => eprintln!("task scheduler panic - cannot show error message due to unknown error type") + } }, Ok(Err(err)) => eprintln!("task scheduler failed - {err:?}"), Ok(Ok(_)) => {} diff --git a/src/tools/parallel_handler.rs b/src/tools/parallel_handler.rs index 17f70179..429a1f8b 100644 --- a/src/tools/parallel_handler.rs +++ b/src/tools/parallel_handler.rs @@ -140,7 +140,13 @@ impl<I: Send + 'static> ParallelHandler<I> { "thread {} ({}) panicked: {}", self.name, i, panic_msg )), - Err(_) => msg_list.push(format!("thread {} ({}) panicked", self.name, i)), + Err(panic) => match panic.downcast::<String>() { + Ok(panic_msg) => msg_list.push(format!( + "thread {} ({}) panicked: {}", + self.name, i, panic_msg + )), + Err(_) => msg_list.push(format!("thread {} ({}) panicked", self.name, i)) + }, } } i += 1; -- 2.39.5 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/1] backup-proxy and parallel_handler: Improved errors when panics occur and the panic message is a formatted (not static) string. This worked already for &str literals, but not for Strings. 2025-01-21 9:36 ` [pbs-devel] [PATCH proxmox-backup 1/1] backup-proxy and parallel_handler: Improved errors when panics occur and the panic message is a formatted (not static) string. This worked already for &str literals, but not for Strings Laurențiu Leahu-Vlăducu @ 2025-01-21 10:56 ` Gabriel Goller 2025-01-21 13:14 ` Laurențiu Leahu-Vlăducu 2025-01-21 13:17 ` Shannon Sterz 0 siblings, 2 replies; 6+ messages in thread From: Gabriel Goller @ 2025-01-21 10:56 UTC (permalink / raw) To: Laurențiu Leahu-Vlăducu; +Cc: pbs-devel On 21.01.2025 10:36, Laurențiu Leahu-Vlăducu wrote: >diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs >index ce1be1c0..a7dab4ac 100644 >--- a/src/bin/proxmox-backup-proxy.rs >+++ b/src/bin/proxmox-backup-proxy.rs >@@ -424,7 +424,10 @@ async fn run_task_scheduler() { > match schedule_tasks().catch_unwind().await { > Err(panic) => match panic.downcast::<&str>() { > Ok(msg) => eprintln!("task scheduler panic: {msg}"), >- Err(_) => eprintln!("task scheduler panic - unknown type"), >+ Err(panic) => match panic.downcast::<String>() { >+ Ok(msg) => eprintln!("task scheduler panic: {msg}"), >+ Err(_) => eprintln!("task scheduler panic - cannot show error message due to unknown error type") AFAIK this gets called after the logging init, so we can use tracing::error for the log messages here. >+ } > }, > Ok(Err(err)) => eprintln!("task scheduler failed - {err:?}"), > Ok(Ok(_)) => {} >diff --git a/src/tools/parallel_handler.rs b/src/tools/parallel_handler.rs >index 17f70179..429a1f8b 100644 >--- a/src/tools/parallel_handler.rs >+++ b/src/tools/parallel_handler.rs >@@ -140,7 +140,13 @@ impl<I: Send + 'static> ParallelHandler<I> { > "thread {} ({}) panicked: {}", > self.name, i, panic_msg > )), >- Err(_) => msg_list.push(format!("thread {} ({}) panicked", self.name, i)), >+ Err(panic) => match panic.downcast::<String>() { >+ Ok(panic_msg) => msg_list.push(format!( >+ "thread {} ({}) panicked: {}", >+ self.name, i, panic_msg >+ )), >+ Err(_) => msg_list.push(format!("thread {} ({}) panicked", self.name, i)) A few variables can be be inlined here. The downcast part looks ok to me. Please keep the commit message title under 72 characters. Thanks! _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/1] backup-proxy and parallel_handler: Improved errors when panics occur and the panic message is a formatted (not static) string. This worked already for &str literals, but not for Strings. 2025-01-21 10:56 ` Gabriel Goller @ 2025-01-21 13:14 ` Laurențiu Leahu-Vlăducu 2025-01-21 13:17 ` Shannon Sterz 1 sibling, 0 replies; 6+ messages in thread From: Laurențiu Leahu-Vlăducu @ 2025-01-21 13:14 UTC (permalink / raw) To: Gabriel Goller; +Cc: pbs-devel Thanks for the review! I sent a v2 which includes your suggestions. On 21.01.25 11:56, Gabriel Goller wrote: > On 21.01.2025 10:36, Laurențiu Leahu-Vlăducu wrote: >> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup- >> proxy.rs >> index ce1be1c0..a7dab4ac 100644 >> --- a/src/bin/proxmox-backup-proxy.rs >> +++ b/src/bin/proxmox-backup-proxy.rs >> @@ -424,7 +424,10 @@ async fn run_task_scheduler() { >> match schedule_tasks().catch_unwind().await { >> Err(panic) => match panic.downcast::<&str>() { >> Ok(msg) => eprintln!("task scheduler panic: {msg}"), >> - Err(_) => eprintln!("task scheduler panic - unknown >> type"), >> + Err(panic) => match panic.downcast::<String>() { >> + Ok(msg) => eprintln!("task scheduler panic: {msg}"), >> + Err(_) => eprintln!("task scheduler panic - >> cannot show error message due to unknown error type") > > AFAIK this gets called after the logging init, so we can use > tracing::error for the log messages here. > >> + } >> }, >> Ok(Err(err)) => eprintln!("task scheduler failed - {err:?}"), >> Ok(Ok(_)) => {} >> diff --git a/src/tools/parallel_handler.rs b/src/tools/ >> parallel_handler.rs >> index 17f70179..429a1f8b 100644 >> --- a/src/tools/parallel_handler.rs >> +++ b/src/tools/parallel_handler.rs >> @@ -140,7 +140,13 @@ impl<I: Send + 'static> ParallelHandler<I> { >> "thread {} ({}) panicked: {}", >> self.name, i, panic_msg >> )), >> - Err(_) => msg_list.push(format!("thread {} ({}) >> panicked", self.name, i)), >> + Err(panic) => match panic.downcast::<String>() { >> + Ok(panic_msg) => msg_list.push(format!( >> + "thread {} ({}) panicked: {}", >> + self.name, i, panic_msg >> + )), >> + Err(_) => msg_list.push(format!("thread {} >> ({}) panicked", self.name, i)) > > A few variables can be be inlined here. > > The downcast part looks ok to me. > Please keep the commit message title under 72 characters. > > Thanks! _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/1] backup-proxy and parallel_handler: Improved errors when panics occur and the panic message is a formatted (not static) string. This worked already for &str literals, but not for Strings. 2025-01-21 10:56 ` Gabriel Goller 2025-01-21 13:14 ` Laurențiu Leahu-Vlăducu @ 2025-01-21 13:17 ` Shannon Sterz 1 sibling, 0 replies; 6+ messages in thread From: Shannon Sterz @ 2025-01-21 13:17 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Laurențiu Leahu-Vlăducu On Tue Jan 21, 2025 at 11:56 AM CET, Gabriel Goller wrote: > On 21.01.2025 10:36, Laurențiu Leahu-Vlăducu wrote: > >diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs > >index ce1be1c0..a7dab4ac 100644 > >--- a/src/bin/proxmox-backup-proxy.rs > >+++ b/src/bin/proxmox-backup-proxy.rs > >@@ -424,7 +424,10 @@ async fn run_task_scheduler() { > > match schedule_tasks().catch_unwind().await { > > Err(panic) => match panic.downcast::<&str>() { > > Ok(msg) => eprintln!("task scheduler panic: {msg}"), > >- Err(_) => eprintln!("task scheduler panic - unknown type"), > >+ Err(panic) => match panic.downcast::<String>() { > >+ Ok(msg) => eprintln!("task scheduler panic: {msg}"), > >+ Err(_) => eprintln!("task scheduler panic - cannot show error message due to unknown error type") > > AFAIK this gets called after the logging init, so we can use > tracing::error for the log messages here. > > >+ } > > }, > > Ok(Err(err)) => eprintln!("task scheduler failed - {err:?}"), > > Ok(Ok(_)) => {} > >diff --git a/src/tools/parallel_handler.rs b/src/tools/parallel_handler.rs > >index 17f70179..429a1f8b 100644 > >--- a/src/tools/parallel_handler.rs > >+++ b/src/tools/parallel_handler.rs > >@@ -140,7 +140,13 @@ impl<I: Send + 'static> ParallelHandler<I> { > > "thread {} ({}) panicked: {}", > > self.name, i, panic_msg > > )), > >- Err(_) => msg_list.push(format!("thread {} ({}) panicked", self.name, i)), > >+ Err(panic) => match panic.downcast::<String>() { > >+ Ok(panic_msg) => msg_list.push(format!( > >+ "thread {} ({}) panicked: {}", > >+ self.name, i, panic_msg > >+ )), > >+ Err(_) => msg_list.push(format!("thread {} ({}) panicked", self.name, i)) > > A few variables can be be inlined here. > > The downcast part looks ok to me. > Please keep the commit message title under 72 characters. small note: for pbs this is actually 70 chars, guess there might have been a wikipage that wasn't updated [1]. [1]: https://pbs.proxmox.com/wiki/index.php/Developer_Documentation#Commits_and_Commit_Messages > > Thanks! > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [pbs-devel] [PATCH proxmox 1/1] rest-server: Improved errors when panics occur and the panic message is a formatted (not static) string. This worked already for &str literals, but not for Strings. 2025-01-21 9:36 [pbs-devel] [PATCH proxmox/proxmox-backup 0/1] Improve errors when panics occur and the panic message is a formatted (not static) string Laurențiu Leahu-Vlăducu 2025-01-21 9:36 ` [pbs-devel] [PATCH proxmox-backup 1/1] backup-proxy and parallel_handler: Improved errors when panics occur and the panic message is a formatted (not static) string. This worked already for &str literals, but not for Strings Laurențiu Leahu-Vlăducu @ 2025-01-21 9:36 ` Laurențiu Leahu-Vlăducu 1 sibling, 0 replies; 6+ messages in thread From: Laurențiu Leahu-Vlăducu @ 2025-01-21 9:36 UTC (permalink / raw) To: pbs-devel; +Cc: Laurențiu Leahu-Vlăducu Downcasting to both &str and String is also done by the Rust Standard Library in the default panic handler. See: https://github.com/rust-lang/rust/blob/b605c65b6eb5fa71783f8e26df69975f9f1680ee/library/std/src/panicking.rs#L777 Signed-off-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com> --- proxmox-rest-server/src/worker_task.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs index beec691e..45ee4e34 100644 --- a/proxmox-rest-server/src/worker_task.rs +++ b/proxmox-rest-server/src/worker_task.rs @@ -982,7 +982,10 @@ impl WorkerTask { Ok(r) => r, Err(panic) => match panic.downcast::<&str>() { Ok(panic_msg) => Err(format_err!("worker panicked: {}", panic_msg)), - Err(_) => Err(format_err!("worker panicked: unknown type.")), + Err(panic) => match panic.downcast::<String>() { + Ok(panic_msg) => Err(format_err!("worker panicked: {}", panic_msg)), + Err(_) => Err(format_err!("worker panicked: cannot show error message due to unknown error type.")) + }, }, }; -- 2.39.5 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-01-21 13:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-01-21 9:36 [pbs-devel] [PATCH proxmox/proxmox-backup 0/1] Improve errors when panics occur and the panic message is a formatted (not static) string Laurențiu Leahu-Vlăducu 2025-01-21 9:36 ` [pbs-devel] [PATCH proxmox-backup 1/1] backup-proxy and parallel_handler: Improved errors when panics occur and the panic message is a formatted (not static) string. This worked already for &str literals, but not for Strings Laurențiu Leahu-Vlăducu 2025-01-21 10:56 ` Gabriel Goller 2025-01-21 13:14 ` Laurențiu Leahu-Vlăducu 2025-01-21 13:17 ` Shannon Sterz 2025-01-21 9:36 ` [pbs-devel] [PATCH proxmox 1/1] rest-server: " Laurențiu Leahu-Vlăducu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox