* [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
* [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
* 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
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