From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 8E40973F29 for ; Tue, 31 May 2022 10:38:39 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 79EEC63FD for ; Tue, 31 May 2022 10:38:09 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 85CD963F3 for ; Tue, 31 May 2022 10:38:08 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 5BFC64221A for ; Tue, 31 May 2022 10:38:02 +0200 (CEST) Date: Tue, 31 May 2022 10:38:00 +0200 From: Wolfgang Bumiller To: Hannes Laimer Cc: pbs-devel@lists.proxmox.com Message-ID: <20220531083712.pzolifsv5dwfezg3@casey.proxmox.com> References: <20220510090158.33504-1-h.laimer@proxmox.com> <20220510090158.33504-4-h.laimer@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220510090158.33504-4-h.laimer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.329 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH proxmox-backup rebased 2/9] pbs-client: replace print with log macro X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 31 May 2022 08:38:39 -0000 On Tue, May 10, 2022 at 09:01:51AM +0000, Hannes Laimer wrote: > --- a/pbs-client/src/backup_writer.rs > +++ b/pbs-client/src/backup_writer.rs > @@ -335,23 +332,19 @@ impl BackupWriter { > None > }, > options.compress, > - self.verbose, > ) > .await?; > > let size_dirty = upload_stats.size - upload_stats.size_reused; > let size: HumanByte = upload_stats.size.into(); > - let archive = if self.verbose { > - archive_name > - } else { > - pbs_tools::format::strip_server_file_extension(archive_name) > - }; > + let archive = pbs_tools::format::strip_server_file_extension(archive_name); I think it would be better to keep the conditional naming using `log::log_enabled!(log::Level::Debug)` as condition > if archive_name != CATALOG_NAME { > let speed: HumanByte = > ((size_dirty * 1_000_000) / (upload_stats.duration.as_micros() as usize)).into(); > let size_dirty: HumanByte = size_dirty.into(); > let size_compressed: HumanByte = upload_stats.size_compressed.into(); > - println!( > + log::debug!("{}", archive_name); ^ Then we don't need the extra line here. > + log::info!( > "{}: had to backup {} of {} (compressed {}) in {:.2}s", > archive, > size_dirty, > @@ -359,32 +352,32 @@ impl BackupWriter { > size_compressed, > upload_stats.duration.as_secs_f64() > ); > - println!("{}: average backup speed: {}/s", archive, speed); > + log::info!("{}: average backup speed: {}/s", archive, speed); > } else { > - println!("Uploaded backup catalog ({})", size); > + log::info!("Uploaded backup catalog ({})", size); > } > > if upload_stats.size_reused > 0 && upload_stats.size > 1024 * 1024 { > let reused_percent = upload_stats.size_reused as f64 * 100. / upload_stats.size as f64; > let reused: HumanByte = upload_stats.size_reused.into(); > - println!( > + log::info!( > "{}: backup was done incrementally, reused {} ({:.1}%)", > archive, reused, reused_percent > ); > } > - if self.verbose && upload_stats.chunk_count > 0 { ^ Could check `log_enabled!` here as well. > - println!( > + if upload_stats.chunk_count > 0 { > + log::debug!( > "{}: Reused {} from {} chunks.", > - archive, upload_stats.chunk_reused, upload_stats.chunk_count > + archive_name, upload_stats.chunk_reused, upload_stats.chunk_count ^ not sure why we would want to change the `archive` to `archive_name` here? > ); > - println!( > + log::debug!( > "{}: Average chunk size was {}.", > - archive, > + archive_name, > HumanByte::from(upload_stats.size / upload_stats.chunk_count) > ); > - println!( > + log::debug!( > "{}: Average time per request: {} microseconds.", > - archive, > + archive_name, > (upload_stats.duration.as_micros()) / (upload_stats.chunk_count as u128) > ); > } (...) > diff --git a/pbs-client/src/pxar/fuse.rs b/pbs-client/src/pxar/fuse.rs > index 594930ef..27e741c1 100644 > --- a/pbs-client/src/pxar/fuse.rs > +++ b/pbs-client/src/pxar/fuse.rs > @@ -240,8 +240,8 @@ impl SessionImpl { > > /// Here's how we deal with errors: > /// > - /// Any error will be printed if the verbose flag was set, otherwise the message will be > - /// silently dropped. > + /// Any error will be logged if a log level of at least 'debug' was set, otherwise the > + /// message will be silently dropped. > /// > /// Opaque errors will cause the fuse main loop to bail out with that error. > /// > @@ -255,8 +255,8 @@ impl SessionImpl { > ) { > let final_result = match err.downcast::() { > Ok(err) => { > - if err.kind() == io::ErrorKind::Other && self.verbose { > - eprintln!("an IO error occurred: {}", err); > + if err.kind() == io::ErrorKind::Other { > + log::debug!("an IO error occurred: {}", err); ^ I think this can be `log::error` > } > > // fail the request > @@ -264,9 +264,7 @@ impl SessionImpl { > } > Err(err) => { > // `bail` (non-`io::Error`) is used for fatal errors which should actually cancel: > - if self.verbose { > - eprintln!("internal error: {}, bailing out", err); > - } > + log::debug!("internal error: {}, bailing out", err); ^ I think this can be `log::error` > Err(err) > } > }; > @@ -385,9 +383,7 @@ impl SessionImpl { > } > } > other => { > - if self.verbose { > - eprintln!("Received unexpected fuse request"); > - } > + log::debug!("Received unexpected fuse request"); ^ I think this can be `log::error` > other.fail(libc::ENOSYS).map_err(Error::from) > } > };