public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH proxmox/proxmox-backup v2 0/1] Improved panic errors with formatted strings
@ 2025-01-21 13:11 Laurențiu Leahu-Vlăducu
  2025-01-21 13:11 ` [pbs-devel] [PATCH proxmox-backup v2 1/1] proxy/parallel_handler: " Laurențiu Leahu-Vlăducu
  2025-01-21 13:11 ` [pbs-devel] [PATCH proxmox v2 1/1] rest-server: " Laurențiu Leahu-Vlăducu
  0 siblings, 2 replies; 5+ messages in thread
From: Laurențiu Leahu-Vlăducu @ 2025-01-21 13:11 UTC (permalink / raw)
  To: pbs-devel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [pbs-devel] [PATCH proxmox-backup v2 1/1] proxy/parallel_handler: Improved panic errors with formatted strings
  2025-01-21 13:11 [pbs-devel] [PATCH proxmox/proxmox-backup v2 0/1] Improved panic errors with formatted strings Laurențiu Leahu-Vlăducu
@ 2025-01-21 13:11 ` Laurențiu Leahu-Vlăducu
  2025-01-24  8:42   ` Thomas Lamprecht
  2025-01-21 13:11 ` [pbs-devel] [PATCH proxmox v2 1/1] rest-server: " Laurențiu Leahu-Vlăducu
  1 sibling, 1 reply; 5+ messages in thread
From: Laurențiu Leahu-Vlăducu @ 2025-01-21 13:11 UTC (permalink / raw)
  To: pbs-devel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [pbs-devel] [PATCH proxmox v2 1/1] rest-server: Improved panic errors with formatted strings
  2025-01-21 13:11 [pbs-devel] [PATCH proxmox/proxmox-backup v2 0/1] Improved panic errors with formatted strings Laurențiu Leahu-Vlăducu
  2025-01-21 13:11 ` [pbs-devel] [PATCH proxmox-backup v2 1/1] proxy/parallel_handler: " Laurențiu Leahu-Vlăducu
@ 2025-01-21 13:11 ` Laurențiu Leahu-Vlăducu
  1 sibling, 0 replies; 5+ messages in thread
From: Laurențiu Leahu-Vlăducu @ 2025-01-21 13:11 UTC (permalink / raw)
  To: pbs-devel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup v2 1/1] proxy/parallel_handler: Improved panic errors with formatted strings
  2025-01-21 13:11 ` [pbs-devel] [PATCH proxmox-backup v2 1/1] proxy/parallel_handler: " Laurențiu Leahu-Vlăducu
@ 2025-01-24  8:42   ` Thomas Lamprecht
  2025-01-24 15:30     ` Laurențiu Leahu-Vlăducu
  0 siblings, 1 reply; 5+ messages in thread
From: Thomas Lamprecht @ 2025-01-24  8:42 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion,
	Laurențiu Leahu-Vlăducu

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup v2 1/1] proxy/parallel_handler: Improved panic errors with formatted strings
  2025-01-24  8:42   ` Thomas Lamprecht
@ 2025-01-24 15:30     ` Laurențiu Leahu-Vlăducu
  0 siblings, 0 replies; 5+ messages in thread
From: Laurențiu Leahu-Vlăducu @ 2025-01-24 15:30 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox Backup Server development discussion

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2025-01-24 15:31 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-01-21 13:11 [pbs-devel] [PATCH proxmox/proxmox-backup v2 0/1] Improved panic errors with formatted strings Laurențiu Leahu-Vlăducu
2025-01-21 13:11 ` [pbs-devel] [PATCH proxmox-backup v2 1/1] proxy/parallel_handler: " Laurențiu Leahu-Vlăducu
2025-01-24  8:42   ` Thomas Lamprecht
2025-01-24 15:30     ` Laurențiu Leahu-Vlăducu
2025-01-21 13:11 ` [pbs-devel] [PATCH proxmox v2 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
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal