[pbs-devel] [PATCH proxmox/proxmox-backup v2 0/1] Improved panic errors with formatted strings

[pbs-devel] [PATCH proxmox/proxmox-backup v2 0/1] Improved panic errors with formatted strings

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

Changes since v1:
* use tracing::error instead of eprintln
* inline some variables when formatting


proxmox-backup:

Laurențiu Leahu-Vlăducu (1):
  proxy/parallel_handler: Improved panic errors with formatted strings

 src/bin/proxmox-backup-proxy.rs |  9 ++++++---
 src/tools/parallel_handler.rs   | 10 +++++-----
 2 files changed, 11 insertions(+), 8 deletions(-)


proxmox:

Laurențiu Leahu-Vlăducu (1):
  rest-server: Improved panic errors with formatted strings

 proxmox-rest-server/src/worker_task.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
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 v2 1/1] proxy/parallel_handler: Improved panic errors with formatted strings

[Laurențiu Leahu-Vlăducu]

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.

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

Changes since v1:
* use tracing::error instead of eprintln
* inline some variables when formatting

 src/bin/proxmox-backup-proxy.rs |  9 ++++++---
 src/tools/parallel_handler.rs   | 10 +++++-----
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index ce1be1c0..21602fdc 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -423,10 +423,13 @@ 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"),
+                Ok(msg) => tracing::error!("task scheduler panic: {msg}"),
+                Err(panic) => match panic.downcast::<String>() {
+                    Ok(msg) => tracing::error!("task scheduler panic: {msg}"),
+                    Err(_) => tracing::error!("task scheduler panic - cannot show error message due to unknown error type")
+                }
             },
-            Ok(Err(err)) => eprintln!("task scheduler failed - {err:?}"),
+            Ok(Err(err)) => tracing::error!("task scheduler failed - {err:?}"),
             Ok(Ok(_)) => {}
         }
     }
diff --git a/src/tools/parallel_handler.rs b/src/tools/parallel_handler.rs
index 17f70179..afa0bc97 100644
--- a/src/tools/parallel_handler.rs
+++ b/src/tools/parallel_handler.rs
@@ -136,11 +136,11 @@ impl<I: Send + 'static> ParallelHandler<I> {
         while let Some(handle) = self.handles.pop() {
             if let Err(panic) = handle.join() {
                 match panic.downcast::<&str>() {
-                    Ok(panic_msg) => msg_list.push(format!(
-                        "thread {} ({}) panicked: {}",
-                        self.name, i, panic_msg
-                    )),
-                    Err(_) => msg_list.push(format!("thread {} ({}) panicked", self.name, i)),
+                    Ok(panic_msg) => msg_list.push(format!("thread {} ({i}) panicked: {panic_msg}", self.name)),
+                    Err(panic) => match panic.downcast::<String>() {
+                        Ok(panic_msg) => msg_list.push(format!("thread {} ({i}) panicked: {panic_msg}", self.name)),
+                        Err(_) => msg_list.push(format!("thread {} ({i}) panicked", self.name))
+                    },
                 }
             }
             i += 1;
-- 
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 v2 1/1] proxy/parallel_handler: Improved panic errors with formatted strings

[Thomas Lamprecht]

Thanks, looks OK from functionallity POV, some–mostly style–comments inline.

Am 21.01.25 um 14:11 schrieb Laurențiu Leahu-Vlăducu:
> 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.
> 
> 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

A hint that you switch from eprintln! to tracing::error would be good to
have in the commit message, ideally with the semantic changes, or the lack
thereof, recorded here too.

> 
> Signed-off-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
> ---
> 
> Changes since v1:
> * use tracing::error instead of eprintln
> * inline some variables when formatting
> 
>  src/bin/proxmox-backup-proxy.rs |  9 ++++++---
>  src/tools/parallel_handler.rs   | 10 +++++-----
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
> index ce1be1c0..21602fdc 100644
> --- a/src/bin/proxmox-backup-proxy.rs
> +++ b/src/bin/proxmox-backup-proxy.rs
> @@ -423,10 +423,13 @@ 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"),
> +                Ok(msg) => tracing::error!("task scheduler panic: {msg}"),
> +                Err(panic) => match panic.downcast::<String>() {
> +                    Ok(msg) => tracing::error!("task scheduler panic: {msg}"),
> +                    Err(_) => tracing::error!("task scheduler panic - cannot show error message due to unknown error type")


Can be ok that way, but maybe using an if/else chain like the linked example
makes this slightly easier to follow when reading? Nested matches that are
basically an else branch look IMO often more complex than what they are,
certainly somewhat subjective though.

Err(panic) => {
    if let Some(msg) = panic.downcast_ref::<&str>() {
        tracing::error!("task scheduler panic: {msg}");
    } else if let Some(s) = panic.downcast_ref::<String>() {
        tracing::error!("task scheduler panic: {msg}");
    } else {
        tracing::error!("task scheduler panic - cannot show error message due to unknown error type")
    }
}

If we have even more occurrences than the three you fix in this series, it might
be worth adding a helper fn for this.

> +                }
>              },
> -            Ok(Err(err)) => eprintln!("task scheduler failed - {err:?}"),
> +            Ok(Err(err)) => tracing::error!("task scheduler failed - {err:?}"),
>              Ok(Ok(_)) => {}
>          }
>      }
> diff --git a/src/tools/parallel_handler.rs b/src/tools/parallel_handler.rs
> index 17f70179..afa0bc97 100644
> --- a/src/tools/parallel_handler.rs
> +++ b/src/tools/parallel_handler.rs
> @@ -136,11 +136,11 @@ impl<I: Send + 'static> ParallelHandler<I> {
>          while let Some(handle) = self.handles.pop() {
>              if let Err(panic) = handle.join() {
>                  match panic.downcast::<&str>() {
> -                    Ok(panic_msg) => msg_list.push(format!(
> -                        "thread {} ({}) panicked: {}",
> -                        self.name, i, panic_msg
> -                    )),
> -                    Err(_) => msg_list.push(format!("thread {} ({}) panicked", self.name, i)),
> +                    Ok(panic_msg) => msg_list.push(format!("thread {} ({i}) panicked: {panic_msg}", self.name)),
> +                    Err(panic) => match panic.downcast::<String>() {
> +                        Ok(panic_msg) => msg_list.push(format!("thread {} ({i}) panicked: {panic_msg}", self.name)),
> +                        Err(_) => msg_list.push(format!("thread {} ({i}) panicked", self.name))
> +                    },
>                  }
>              }
>              i += 1;



_______________________________________________
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 v2 1/1] proxy/parallel_handler: Improved panic errors with formatted strings

[Laurențiu Leahu-Vlăducu]

Thanks for the review! I fully agree with your suggestions and sent a v3 
that contains them.

On 24.01.25 09:42, Thomas Lamprecht wrote:
> Thanks, looks OK from functionallity POV, some–mostly style–comments inline.
> 
> Am 21.01.25 um 14:11 schrieb Laurențiu Leahu-Vlăducu:
>> 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.
>>
>> 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
> 
> A hint that you switch from eprintln! to tracing::error would be good to
> have in the commit message, ideally with the semantic changes, or the lack
> thereof, recorded here too.
> 
>>
>> Signed-off-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
>> ---
>>
>> Changes since v1:
>> * use tracing::error instead of eprintln
>> * inline some variables when formatting
>>
>>   src/bin/proxmox-backup-proxy.rs |  9 ++++++---
>>   src/tools/parallel_handler.rs   | 10 +++++-----
>>   2 files changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
>> index ce1be1c0..21602fdc 100644
>> --- a/src/bin/proxmox-backup-proxy.rs
>> +++ b/src/bin/proxmox-backup-proxy.rs
>> @@ -423,10 +423,13 @@ 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"),
>> +                Ok(msg) => tracing::error!("task scheduler panic: {msg}"),
>> +                Err(panic) => match panic.downcast::<String>() {
>> +                    Ok(msg) => tracing::error!("task scheduler panic: {msg}"),
>> +                    Err(_) => tracing::error!("task scheduler panic - cannot show error message due to unknown error type")
> 
> 
> Can be ok that way, but maybe using an if/else chain like the linked example
> makes this slightly easier to follow when reading? Nested matches that are
> basically an else branch look IMO often more complex than what they are,
> certainly somewhat subjective though.
> 
> Err(panic) => {
>      if let Some(msg) = panic.downcast_ref::<&str>() {
>          tracing::error!("task scheduler panic: {msg}");
>      } else if let Some(s) = panic.downcast_ref::<String>() {
>          tracing::error!("task scheduler panic: {msg}");
>      } else {
>          tracing::error!("task scheduler panic - cannot show error message due to unknown error type")
>      }
> }
> 
> If we have even more occurrences than the three you fix in this series, it might
> be worth adding a helper fn for this.
> 
>> +                }
>>               },
>> -            Ok(Err(err)) => eprintln!("task scheduler failed - {err:?}"),
>> +            Ok(Err(err)) => tracing::error!("task scheduler failed - {err:?}"),
>>               Ok(Ok(_)) => {}
>>           }
>>       }
>> diff --git a/src/tools/parallel_handler.rs b/src/tools/parallel_handler.rs
>> index 17f70179..afa0bc97 100644
>> --- a/src/tools/parallel_handler.rs
>> +++ b/src/tools/parallel_handler.rs
>> @@ -136,11 +136,11 @@ impl<I: Send + 'static> ParallelHandler<I> {
>>           while let Some(handle) = self.handles.pop() {
>>               if let Err(panic) = handle.join() {
>>                   match panic.downcast::<&str>() {
>> -                    Ok(panic_msg) => msg_list.push(format!(
>> -                        "thread {} ({}) panicked: {}",
>> -                        self.name, i, panic_msg
>> -                    )),
>> -                    Err(_) => msg_list.push(format!("thread {} ({}) panicked", self.name, i)),
>> +                    Ok(panic_msg) => msg_list.push(format!("thread {} ({i}) panicked: {panic_msg}", self.name)),
>> +                    Err(panic) => match panic.downcast::<String>() {
>> +                        Ok(panic_msg) => msg_list.push(format!("thread {} ({i}) panicked: {panic_msg}", self.name)),
>> +                        Err(_) => msg_list.push(format!("thread {} ({i}) panicked", self.name))
>> +                    },
>>                   }
>>               }
>>               i += 1;
> 



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox v2 1/1] rest-server: Improved panic errors with formatted strings

[Laurențiu Leahu-Vlăducu]

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.

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>
---
Changes since v1:
* inline some variables when formatting

 proxmox-rest-server/src/worker_task.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
index beec691e..a4dc35fe 100644
--- a/proxmox-rest-server/src/worker_task.rs
+++ b/proxmox-rest-server/src/worker_task.rs
@@ -981,8 +981,11 @@ impl WorkerTask {
                     let result = match std::panic::catch_unwind(move || f(worker1)) {
                         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.")),
+                            Ok(panic_msg) => Err(format_err!("worker panicked: {panic_msg}")),
+                            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