Re: [pbs-devel] [PATCH backup] inspect: don't panic when writing to a broken pipe

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Mon, Apr 14, 2025 at 04:02:44PM +0200, Maximiliano Sandoval wrote:
> println [panics] if writing to stdout fails. This can be reproduced by
> calling
> 
> ```
> proxmox-backup-debug inspect file {FIXED_INDEX_FILE} | head
> ```
> 
> for example.
> 
> [panics] https://doc.rust-lang.org/std/macro.println.html#panics
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
> 
> This raises the question of where else the same change should be done and
> whether it is worth it. Imho wanting to see the first 10 or so chunks of an
> index file for debugging purposes should come often enough that this should be
> fine here.

It's a debugging tool with the purpose of printing stuff and if stdout
goes away you lose your main functionality.
So is it really worth caring about this? If you don't like the output
here, we may as well just wrap `main()` in a `catch_unwind()`?

As for other tools - we'll see when we run into it.
I'd argue that in cases where this is "actually wrong" printing to
stdout was the wrong choice in the first place.
So this is only a problem where we "log by stdout/err", which should
probably be addressed by changing to a log call? Would need to check
the actual cases.

> 
>  src/bin/proxmox_backup_debug/inspect.rs | 70 ++++++++++++++++---------
>  1 file changed, 44 insertions(+), 26 deletions(-)
> 
> diff --git a/src/bin/proxmox_backup_debug/inspect.rs b/src/bin/proxmox_backup_debug/inspect.rs
> index 321080be3..c75871da9 100644
> --- a/src/bin/proxmox_backup_debug/inspect.rs
> +++ b/src/bin/proxmox_backup_debug/inspect.rs
> @@ -203,16 +203,21 @@ fn inspect_chunk(
>      };
>  
>      if output_format == "text" {
> -        println!("CRC: {}", val["crc"]);
> -        println!("encryption: {}", val["encryption"]);
> -        println!("is-compressed: {}", val["is-compressed"]);
> -        println!("size: {}", val["size"]);
> -        if let Some(refs) = val["referenced-by"].as_array() {
> -            println!("referenced by:");
> -            for reference in refs {
> -                println!("  {}", reference);
> +        fn print_to_stdout(val: Value) -> std::io::Result<()> {
> +            let mut stdout = std::io::stdout();
> +            writeln!(&mut stdout, "CRC: {}", val["crc"])?;
> +            writeln!(&mut stdout, "encryption: {}", val["encryption"])?;
> +            writeln!(&mut stdout, "is-compressed: {}", val["is-compressed"])?;
> +            writeln!(&mut stdout, "size: {}", val["size"])?;
> +            if let Some(refs) = val["referenced-by"].as_array() {
> +                writeln!(&mut stdout, "referenced by:")?;
> +                for reference in refs {
> +                    writeln!(&mut stdout, "  {}", reference)?;
> +                }
>              }
> +            Ok(())
>          }
> +        let _ = print_to_stdout(val);
>      } else {
>          format_and_print_result(&val, &output_format);
>      }
> @@ -311,19 +316,25 @@ fn inspect_file(
>      };
>  
>      if output_format == "text" {
> -        println!("size: {}", val["size"]);
> -        if let Some(encryption) = val["encryption"].as_str() {
> -            println!("encryption: {}", encryption);
> -        }
> -        if let Some(ctime) = val["ctime"].as_str() {
> -            println!("creation time: {}", ctime);
> -        }
> -        if let Some(chunks) = val["chunk-digests"].as_array() {
> -            println!("chunks:");
> -            for chunk in chunks {
> -                println!("  {}", chunk);
> +        fn print_to_stdout(val: Value) -> std::io::Result<()> {
> +            let mut stdout = std::io::stdout();
> +            writeln!(&mut stdout, "size: {}", val["size"])?;
> +            if let Some(encryption) = val["encryption"].as_str() {
> +                writeln!(&mut stdout, "encryption: {}", encryption)?;
>              }
> +            if let Some(ctime) = val["ctime"].as_str() {
> +                writeln!(&mut stdout, "creation time: {}", ctime)?;
> +            }
> +            if let Some(chunks) = val["chunk-digests"].as_array() {
> +                writeln!(&mut stdout, "chunks:")?;
> +                for chunk in chunks {
> +                    writeln!(&mut stdout, "  {}", chunk)?;
> +                }
> +            }
> +            Ok(())
>          }
> +
> +        let _ = print_to_stdout(val);
>      } else {
>          format_and_print_result(&val, &output_format);
>      }
> @@ -458,14 +469,21 @@ fn inspect_device(device: String, param: Value) -> Result<(), Error> {
>      std::fs::remove_dir(std::path::Path::new(&tmp_mount_path))?;
>  
>      if output_format == "text" {
> -        println!("Device contains {ds_count} stores");
> -        println!("---------------");
> -        for s in stores {
> -            println!(
> -                "Datastore at {} | VM: {}, CT: {}, HOST: {}, NS: {}",
> -                s["path"], s["vm-count"], s["ct-count"], s["host-count"], s["ns-count"]
> -            );
> +        fn print_to_stdout(ds_count: i32, stores: &[Value]) -> std::io::Result<()> {
> +            let mut stdout = std::io::stdout();
> +
> +            writeln!(&mut stdout, "Device contains {ds_count} stores")?;
> +            writeln!(&mut stdout, "---------------")?;
> +            for s in stores {
> +                writeln!(
> +                    &mut stdout,
> +                    "Datastore at {} | VM: {}, CT: {}, HOST: {}, NS: {}",
> +                    s["path"], s["vm-count"], s["ct-count"], s["host-count"], s["ns-count"]
> +                )?;
> +            }
> +            Ok(())
>          }
> +        let _ = print_to_stdout(ds_count, &stores);
>      } else {
>          format_and_print_result(
>              &json!({"store_count": stores.len(), "stores": stores}),
> -- 
> 2.39.5


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