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 4893178661 for ; Fri, 30 Apr 2021 11:40:12 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2826725E06 for ; Fri, 30 Apr 2021 11:39:42 +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 8934B25DF4 for ; Fri, 30 Apr 2021 11:39:40 +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 59D8C464B8 for ; Fri, 30 Apr 2021 11:39:40 +0200 (CEST) Date: Fri, 30 Apr 2021 11:39:38 +0200 From: Wolfgang Bumiller To: Hannes Laimer Cc: pbs-devel@lists.proxmox.com Message-ID: <20210430093938.3elqtkw6th6ix3e2@wobu-vie.proxmox.com> References: <20210429110016.1467670-1-h.laimer@proxmox.com> <20210429110016.1467670-2-h.laimer@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210429110016.1467670-2-h.laimer@proxmox.com> User-Agent: NeoMutt/20180716 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.022 Adjusted score from AWL reputation of From: address 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 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, tools.rs, mod.rs] Subject: Re: [pbs-devel] [PATCH v5 proxmox-backup 1/4] add chunk inspection to pb-debug 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: Fri, 30 Apr 2021 09:40:12 -0000 On Thu, Apr 29, 2021 at 01:00:13PM +0200, Hannes Laimer wrote: > diff --git a/src/bin/proxmox_backup_debug/inspect.rs b/src/bin/proxmox_backup_debug/inspect.rs > new file mode 100644 > index 00000000..dd6ee287 > --- /dev/null > +++ b/src/bin/proxmox_backup_debug/inspect.rs > @@ -0,0 +1,162 @@ > +use std::path::Path; > + > +use anyhow::{format_err, Error}; > +use proxmox::api::cli::{ > + format_and_print_result, get_output_format, CliCommand, CliCommandMap, CommandLineInterface, > +}; > +use proxmox::api::{api, cli::*}; > +use serde_json::{json, Value}; > +use walkdir::WalkDir; > + > +use proxmox_backup::backup::{ > + load_and_decrypt_key, CryptConfig, DataBlob, DynamicIndexReader, FixedIndexReader, IndexFile, > +}; > + > +use crate::{get_encryption_key_password, KEYFILE_SCHEMA, PATH_SCHEMA}; > + > +use proxmox_backup::tools::outfile_or_stdout; > + > +/// Decodes a blob and writes its content either to stdout or into a file > +fn decode_blob( > + output_path: Option<&Path>, > + key_file: Option<&Path>, > + digest: Option<&[u8; 32]>, > + blob: &DataBlob, > +) -> Result<(), Error> { > + let mut crypt_conf_opt = None; > + let crypt_conf; > + > + if blob.is_encrypted() && key_file.is_some() { > + let (key, _created, _fingerprint) = > + load_and_decrypt_key(&key_file.unwrap(), &get_encryption_key_password)?; > + crypt_conf = CryptConfig::new(key)?; > + crypt_conf_opt = Some(&crypt_conf); > + } > + > + outfile_or_stdout(output_path)?.write_all(blob.decode(crypt_conf_opt, digest)?.as_slice())?; > + Ok(()) > +} > + > +#[api( > + input: { > + properties: { > + chunk: { > + schema: PATH_SCHEMA, > + }, > + "reference-filter": { > + schema: PATH_SCHEMA, > + optional: true, > + }, > + "decode": { > + schema: PATH_SCHEMA, > + optional: true, > + }, > + "keyfile": { > + schema: KEYFILE_SCHEMA, > + optional: true, > + }, > + "output-format": { > + schema: OUTPUT_FORMAT, > + optional: true, > + }, > + } > + } > +)] > +/// Inspect a chunk > +fn inspect_chunk( > + chunk: String, > + reference_filter: Option, > + decode: Option, > + keyfile: Option, > + param: Value, > +) -> Result<(), Error> { > + let output_format = get_output_format(¶m); > + let chunk_path = Path::new(&chunk); > + let digest_str = chunk_path.file_name().unwrap().to_str().unwrap(); ^ The first unwrap() fails with an ugly error on `/`, so please don't use unwrap there. Also I think for a debug helper it makes sense if it can also work on copied and renamed files that aren't literally named after their digest, so I think `digest_{str,raw}` should in fact be `Option<>`. (In the `decode` method the digest and its verification are already optional after all.) You can, however, output a warning that the digest will not be verified. (Also I wonder if an additional optional `--digest` parameter should be added?) > + let digest_raw = proxmox::tools::hex_to_digest(digest_str) > + .map_err(|e| format_err!("could not parse chunk - {}", e))?; > + > + let search_path = reference_filter.as_ref().map(Path::new); > + let key_file_path = keyfile.as_ref().map(Path::new); > + > + let (decode_output_path, to_stdout) = ( > + decode.as_ref().map(Path::new), > + decode.clone().map_or(false, |p| p.eq("-")), ^ `to_stdout=true` now implies that `decode_output_path` was `Some`, so I think you can drop that variable altogether. > + ); > + > + let mut file = std::fs::File::open(&chunk_path) > + .map_err(|e| format_err!("could not open chunk file - {}", e))?; > + let blob = DataBlob::load_from_reader(&mut file)?; ^ If you're doing cleanups, I'd still like `file` to be dropped at this point. > + > + let mut referenced_by = None; > + if let Some(search_path) = search_path { Try to avoid `mut` variables where they're only assigned once anyway: let referenced_by = if let Some(search_path) = search_path { ... Some(references) } else { None }; > + let mut references = Vec::new(); > + for entry in WalkDir::new(search_path) > + .follow_links(false) > + .into_iter() > + .filter_map(|e| e.ok()) > + { > + let file_name = entry.file_name().to_string_lossy(); ^ Since you're just throwing away the produced string and only want to check the suffix, just do: use std::os::unix::ffi::OsStrExt; let file_name = entry.file_name().as_bytes(); and prefix the strings with a `b` => `b".fidx"` > + let mut index: Option> = None; Same pattern as above: let index = if file_name.ends_with(b".fidx") { ... } else if file_name.ends_with(b".didx") { ... } else { continue; }; Also note the `continue`: > + > + if file_name.ends_with(".fidx") { > + index = match FixedIndexReader::open(entry.path()) { > + Ok(index) => Some(Box::new(index)), > + Err(_) => None, You can just drop the whole `Some`/`Option` part and use Err(_) => continue, here > + }; > + } > + > + if file_name.ends_with(".didx") { > + index = match DynamicIndexReader::open(entry.path()) { > + Ok(index) => Some(Box::new(index)), > + Err(_) => None, and here > + }; > + } > + > + if let Some(index) = index { and then drop the condition above entirely and get rid of a level of indentation on the block below > + for pos in 0..index.index_count() { > + if let Some(index_chunk_digest) = index.index_digest(pos) { > + if digest_raw.eq(index_chunk_digest) { > + references.push(entry.path().to_string_lossy().into_owned()); > + break; > + } > + } > + } > + } > + } > + referenced_by = Some(references); > + } > + > + if decode_output_path.is_some() || to_stdout { ... because this is now: `if X || ` > + decode_blob(decode_output_path, key_file_path, Some(&digest_raw), &blob)?; > + } > + > + let crc_status = format!( > + "{}({})", > + blob.compute_crc(), > + blob.verify_crc().map_or("NOK", |_| "OK") > + ); > + > + let val = match referenced_by { > + Some(references) => json!({ > + "digest": digest_str, > + "crc": crc_status, > + "encryption": blob.crypt_mode()?, > + "referenced-by": references > + }), > + None => json!({ > + "digest": digest_str, > + "crc": crc_status, > + "encryption": blob.crypt_mode()?, > + }), > + }; > + > + format_and_print_result(&val, &output_format); > + Ok(()) > +} > + > +pub fn inspect_commands() -> CommandLineInterface { > + let cmd_def = CliCommandMap::new().insert("chunk", CliCommand::new(&API_METHOD_INSPECT_CHUNK)); ^ I think this should also get a `.arg_param(["chunk"])`. Otherwise it feels somewhat as if you'd have to write $ cat --file foo instead of just $ cat foo ;-) > + > + cmd_def.into() > +} > diff --git a/src/bin/proxmox_backup_debug/mod.rs b/src/bin/proxmox_backup_debug/mod.rs > new file mode 100644 > index 00000000..644583db > --- /dev/null > +++ b/src/bin/proxmox_backup_debug/mod.rs > @@ -0,0 +1,2 @@ > +mod inspect; > +pub use inspect::*; > diff --git a/src/tools.rs b/src/tools.rs > index 08af55e5..16ebf3fc 100644 > --- a/src/tools.rs > +++ b/src/tools.rs > @@ -6,7 +6,7 @@ use std::borrow::Borrow; > use std::collections::HashMap; > use std::hash::BuildHasher; > use std::fs::File; > -use std::io::{self, BufRead, Read, Seek, SeekFrom}; > +use std::io::{self, BufRead, Read, Seek, SeekFrom, stdout, Write}; > use std::os::unix::io::RawFd; > use std::path::Path; > > @@ -571,3 +571,13 @@ pub fn create_run_dir() -> Result<(), Error> { > let _: bool = proxmox::tools::fs::create_path(PROXMOX_BACKUP_RUN_DIR_M!(), None, None)?; > Ok(()) > } Code below is fine but needs rebasing on current master. > + > +/// Returns either a new file, if a path is given, or stdout, if no path is given. > +pub fn outfile_or_stdout>(path: Option

) -> Result, Error> { > + if let Some(path) = path { > + let f = File::create(path)?; > + Ok(Box::new(f) as Box) > + } else { > + Ok(Box::new(stdout()) as Box) > + } > +} > -- > 2.20.1