Re: [pbs-devel] [PATCH vma-to-pbs v5 3/4] use level-based logging instead of println

["Shannon Sterz" <s.sterz@proxmox.com>]

comments in-line:

On Mon Nov 11, 2024 at 2:08 PM CET, Filip Schauer wrote:
> Use log level "info" by default and prevent spamming messages for every
> single chunk uploaded. To re-enable these messages, set the RUST_LOG
> environment variable to "debug".
>
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  Cargo.toml     |  2 ++
>  src/main.rs    | 28 ++++++++++++++++++++++------
>  src/vma2pbs.rs | 38 ++++++++++++++++++++------------------
>  3 files changed, 44 insertions(+), 24 deletions(-)
>
> diff --git a/Cargo.toml b/Cargo.toml
> index ad80304..7951bbc 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -8,7 +8,9 @@ edition = "2021"
>  anyhow = "1.0"
>  bincode = "1.3"
>  chrono = "0.4"
> +env_logger = "0.10"
>  hyper = "0.14.5"
> +log = "0.4"
>  pico-args = "0.5"
>  md5 = "0.7.0"
>  regex = "1.7"
> diff --git a/src/main.rs b/src/main.rs
> index d4b36fa..203196b 100644
> --- a/src/main.rs
> +++ b/src/main.rs
> @@ -6,6 +6,7 @@ use std::path::PathBuf;
>
>  use anyhow::{bail, Context, Error};
>  use chrono::NaiveDateTime;
> +use env_logger::Target;
>  use proxmox_sys::linux::tty;
>  use proxmox_time::epoch_i64;
>  use regex::Regex;
> @@ -128,7 +129,7 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>
>      match (encrypt, keyfile.is_some()) {
>          (true, false) => bail!("--encrypt requires a --keyfile!"),
> -        (false, true) => println!(
> +        (false, true) => log::info!(
>              "--keyfile given, but --encrypt not set -> backup will be signed, but not encrypted!"
>          ),
>          _ => {}
> @@ -190,7 +191,7 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>
>              Some(key_password)
>          } else if vma_file_path.is_none() {
> -            println!(
> +            log::info!(
>                  "Please use --key-password-file to provide the password when passing the VMA file \
>                  to stdin, if required."
>              );
> @@ -246,13 +247,17 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>                  let Some((_, [backup_id, timestr, ext])) =
>                      re.captures(file_name).map(|c| c.extract())
>                  else {
> -                    // Skip the file, since it is not a VMA backup
> +                    log::debug!("Skip \"{file_name}\", since it is not a VMA backup");
>                      continue;
>                  };
>
>                  if let Some(ref vmid) = vmid {
>                      if backup_id != vmid {
> -                        // Skip the backup, since it does not match the specified vmid
> +                        log::debug!(
> +                            "Skip backup with VMID {}, since it does not match specified VMID {}",
> +                            backup_id,
> +                            vmid

nit: you can use format strings here

> +                        );
>                          continue;
>                      }
>                  }
> @@ -308,14 +313,14 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>              bail!("Did not find any backup archives");
>          }
>
> -        println!(
> +        log::info!(
>              "Found {} backup archive(s) of {} different VMID(s):",
>              total_vma_count,
>              grouped_vmas.len()
>          );
>
>          for (backup_id, vma_group) in &grouped_vmas {
> -            println!("- VMID {}: {} backups", backup_id, vma_group.len());
> +            log::info!("- VMID {}: {} backups", backup_id, vma_group.len());
>          }

nit: if you are already touching this, move this over to format strings
as well

>
>          if !yes {
> @@ -358,7 +363,18 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>      Ok(options)
>  }
>
> +fn init_cli_logger() {
> +    env_logger::Builder::from_env(env_logger::Env::new().filter_or("RUST_LOG", "info"))
> +        .format_level(false)
> +        .format_target(false)
> +        .format_timestamp(None)
> +        .target(Target::Stdout)
> +        .init();
> +}
> +
>  fn main() -> Result<(), Error> {
> +    init_cli_logger();
> +
>      let args = parse_args()?;
>      vma2pbs(args)?;
>
> diff --git a/src/vma2pbs.rs b/src/vma2pbs.rs
> index a5b4027..0517251 100644
> --- a/src/vma2pbs.rs
> +++ b/src/vma2pbs.rs
> @@ -82,8 +82,8 @@ fn create_pbs_backup_task(
>      pbs_args: &PbsArgs,
>      backup_args: &VmaBackupArgs,
>  ) -> Result<*mut ProxmoxBackupHandle, Error> {
> -    println!(
> -        "backup time: {}",
> +    log::info!(
> +        "\tbackup time: {}",
>          epoch_to_rfc3339(backup_args.backup_time)?
>      );
>
> @@ -152,7 +152,7 @@ where
>          let config_name = config.name;
>          let config_data = config.content;
>
> -        println!("CFG: size: {} name: {}", config_data.len(), config_name);
> +        log::info!("\tCFG: size: {} name: {}", config_data.len(), config_name);

nit: move `config_name` into the format string

>
>          let config_name_cstr = CString::new(config_name)?;
>
> @@ -190,9 +190,11 @@ where
>          let device_name = vma_reader.get_device_name(device_id.try_into()?)?;
>          let device_size = vma_reader.get_device_size(device_id.try_into()?)?;
>
> -        println!(
> -            "DEV: dev_id={} size: {} devname: {}",
> -            device_id, device_size, device_name
> +        log::info!(
> +            "\tDEV: dev_id={} size: {} devname: {}",

nit: format string

> +            device_id,
> +            device_size,
> +            device_name
>          );
>
>          let device_name_cstr = CString::new(device_name)?;
> @@ -276,8 +278,8 @@ where
>          };
>
>          let pbs_upload_chunk = |pbs_chunk_buffer: Option<&[u8]>| {
> -            println!(
> -                "Uploading dev_id: {} offset: {:#0X} - {:#0X}",
> +            log::debug!(
> +                "\tUploading dev_id: {} offset: {:#0X} - {:#0X}",

nit: format string, for example:

`\tUploading dev_id: {dev_id} offset: {pbs_chunk_offset:#0X} - {:#0X}`

>                  dev_id,
>                  pbs_chunk_offset,
>                  pbs_chunk_offset + pbs_chunk_size,
> @@ -466,13 +468,13 @@ fn set_notes(
>
>  pub fn vma2pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> {
>      let pbs_args = &args.pbs_args;
> -    println!("PBS repository: {}", pbs_args.pbs_repository);
> +    log::info!("PBS repository: {}", pbs_args.pbs_repository);
>      if let Some(ns) = &pbs_args.namespace {
> -        println!("PBS namespace: {}", ns);
> +        log::info!("PBS namespace: {}", ns);

nit: format string

>      }
> -    println!("PBS fingerprint: {}", pbs_args.fingerprint);
> -    println!("compress: {}", pbs_args.compress);
> -    println!("encrypt: {}", pbs_args.encrypt);
> +    log::info!("PBS fingerprint: {}", pbs_args.fingerprint);
> +    log::info!("compress: {}", pbs_args.compress);
> +    log::info!("encrypt: {}", pbs_args.encrypt);
>
>      let start_transfer_time = SystemTime::now();
>
> @@ -486,8 +488,8 @@ pub fn vma2pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> {
>                  );
>
>                  if args.skip_failed {
> -                    eprintln!("{}", err_msg);
> -                    println!("Skipping VMID {}", backup_args.backup_id);
> +                    log::warn!("{}", err_msg);
> +                    log::info!("Skipping VMID {}", backup_args.backup_id);
>                      break;
>                  } else {
>                      bail!(err_msg);
> @@ -501,15 +503,15 @@ pub fn vma2pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> {
>      let minutes = total_seconds / 60;
>      let seconds = total_seconds % 60;
>      let milliseconds = transfer_duration.as_millis() % 1000;
> -    println!("Backup finished within {minutes} minutes, {seconds} seconds and {milliseconds} ms");
> +    log::info!("Backup finished within {minutes} minutes, {seconds} seconds and {milliseconds} ms");
>
>      Ok(())
>  }
>
>  fn upload_vma_file(pbs_args: &PbsArgs, backup_args: &VmaBackupArgs) -> Result<(), Error> {
>      match &backup_args.vma_file_path {
> -        Some(vma_file_path) => println!("Uploading VMA backup from {:?}", vma_file_path),
> -        None => println!("Uploading VMA backup from (stdin)"),
> +        Some(vma_file_path) => log::info!("Uploading VMA backup from {:?}", vma_file_path),

nit: format string

> +        None => log::info!("Uploading VMA backup from (stdin)"),
>      };
>
>      let vma_file: Box<dyn BufRead> = match &backup_args.compression {



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