* [pbs-devel] [PATCH backup] inspect: don't panic when writing to a broken pipe
@ 2025-04-14 14:02 Maximiliano Sandoval
2025-04-15 6:26 ` Wolfgang Bumiller
0 siblings, 1 reply; 2+ messages in thread
From: Maximiliano Sandoval @ 2025-04-14 14:02 UTC (permalink / raw)
To: pbs-devel
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.
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [pbs-devel] [PATCH backup] inspect: don't panic when writing to a broken pipe
2025-04-14 14:02 [pbs-devel] [PATCH backup] inspect: don't panic when writing to a broken pipe Maximiliano Sandoval
@ 2025-04-15 6:26 ` Wolfgang Bumiller
0 siblings, 0 replies; 2+ messages in thread
From: Wolfgang Bumiller @ 2025-04-15 6:26 UTC (permalink / raw)
To: Maximiliano Sandoval; +Cc: pbs-devel
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-04-15 6:26 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-04-14 14:02 [pbs-devel] [PATCH backup] inspect: don't panic when writing to a broken pipe Maximiliano Sandoval
2025-04-15 6:26 ` Wolfgang Bumiller
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