From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 4ADC31FF172
for <inbox@lore.proxmox.com>; Tue, 15 Apr 2025 08:26:45 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 0D69A3F08E;
Tue, 15 Apr 2025 08:26:44 +0200 (CEST)
Date: Tue, 15 Apr 2025 08:26:39 +0200
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Maximiliano Sandoval <m.sandoval@proxmox.com>
Message-ID: <etyrk4jpzkg6xw4hv6jxxwbhppza3pfvazvpwf4cepapfgiuee@k724wjxuexga>
References: <20250414140244.539303-1-m.sandoval@proxmox.com>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <20250414140244.539303-1-m.sandoval@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.080 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [inspect.rs, rust-lang.org]
Subject: Re: [pbs-devel] [PATCH backup] inspect: don't panic when writing to
a broken pipe
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Cc: pbs-devel@lists.proxmox.com
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pbs-devel-bounces@lists.proxmox.com
Sender: "pbs-devel" <pbs-devel-bounces@lists.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