* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.Service provided by Proxmox Server Solutions GmbH | Privacy | Legal