[pbs-devel] [PATCH proxmox/proxmox-backup 0/1] Improve errors when panics occur and the panic message is a formatted (not static) string

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

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

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

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

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

[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

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.

[Gabriel Goller]

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

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.

[Laurențiu Leahu-Vlăducu]

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

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.

[Shannon Sterz]

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