Re: [pbs-devel] [PATCH v4 proxmox-backup 1/3] add chunk inspection to pb-debug

[Dominik Csapak <d.csapak@proxmox.com>]

comments inline

On 2/19/21 13:09, Hannes Laimer wrote:
> Adds possibility to inspect chunks and find indexes that reference the
> chunk. Options:
>   - chunk: path to the chunk file
>   - [opt] decode: path to a file or to stdout(-), if specified, the chunk will
>     be decoded into the specified location
>   - [opt] keyfile: path to a keyfile, needed if decode is specified and the
>     data was encrypted
>   - [opt] reference-filter: path in which indexes that reference the chunk
>     should be searched, can be a group, snapshot or the whole datastore,
>     if not specified no references will be searched
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> v4:
>   - left the two schemas here since they are quite specific to this binary
>   - output_or_stdout() directly outputs the data instead of returning
>     stdout or an open file (could not find a type that allows to properly
>     return either stdout or a file)

the type would be either
  Box<dyn std::io::Write>
  File(then we'd have to to a File::from_raw_fd to get a File for stdout)
  enum Output {
    File(File),
    Stdout,
  } (then we would have either to match everywhere on that, or
impl write on that, so that we can simply do a write_all on it)

while the function is ok, i think (like Wolfgang) a helper to
get a handle to Option<File> or Option<Box<dyn Write>> or Option<Output>
would be much better than having an output_path and a to_stdout variable

> 
>   Makefile                                |   3 +-
>   src/bin/proxmox-backup-debug.rs         |  32 +++++
>   src/bin/proxmox_backup_debug/inspect.rs | 162 ++++++++++++++++++++++++
>   src/bin/proxmox_backup_debug/mod.rs     |   2 +
>   src/tools.rs                            |  11 +-
>   5 files changed, 208 insertions(+), 2 deletions(-)
>   create mode 100644 src/bin/proxmox-backup-debug.rs
>   create mode 100644 src/bin/proxmox_backup_debug/inspect.rs
>   create mode 100644 src/bin/proxmox_backup_debug/mod.rs
> 
> diff --git a/Makefile b/Makefile
> index b2ef9d32..120e7f98 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -15,7 +15,8 @@ USR_BIN := \
>   
>   # Binaries usable by admins
>   USR_SBIN := \
> -	proxmox-backup-manager
> +	proxmox-backup-manager \
> +	proxmox-backup-debug \
>   
>   # Binaries for services:
>   SERVICE_BIN := \
> diff --git a/src/bin/proxmox-backup-debug.rs b/src/bin/proxmox-backup-debug.rs
> new file mode 100644
> index 00000000..ae747c5d
> --- /dev/null
> +++ b/src/bin/proxmox-backup-debug.rs
> @@ -0,0 +1,32 @@
> +use anyhow::Error;
> +
> +use proxmox::api::cli::*;
> +use proxmox::api::schema::{Schema, StringSchema};
> +use proxmox::sys::linux::tty;
> +
> +mod proxmox_backup_debug;
> +use proxmox_backup_debug::inspect_commands;
> +
> +pub fn get_encryption_key_password() -> Result<Vec<u8>, Error> {
> +    tty::read_password("Encryption Key Password: ")
> +}
> +
> +pub const PATH_SCHEMA: Schema = StringSchema::new("Path to a file or a directory").schema();
> +
> +pub const KEYFILE_SCHEMA: Schema = StringSchema::new(
> +    "Path to encryption key. If the data was encrypted, this key will be used for decryption.",
> +)
> +.schema();

at least this we could reuse from the proxmox_client_tools, no ?
if there is a more specific reason, please state so at least in a 
comment. (proxmox-file-restore also uses the proxmox_client_tools for 
example)

> +
> +fn main() {
> +    proxmox_backup::tools::setup_safe_path_env();
> +
> +    let cmd_def = CliCommandMap::new().insert("inspect", inspect_commands());
> +
> +    let rpcenv = CliEnvironment::new();
> +    run_cli_command(
> +        cmd_def,
> +        rpcenv,
> +        Some(|future| proxmox_backup::tools::runtime::main(future)),
> +    );
> +}
> diff --git a/src/bin/proxmox_backup_debug/inspect.rs b/src/bin/proxmox_backup_debug/inspect.rs
> new file mode 100644
> index 00000000..b211857e
> --- /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::output_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);
> +    }
> +
> +    output_or_stdout(output_path, blob.decode(crypt_conf_opt, digest)?.as_slice())
> +}
> +
> +#[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<String>,
> +    decode: Option<String>,
> +    keyfile: Option<String>,
> +    param: Value,
> +) -> Result<(), Error> {
> +    let output_format = get_output_format(&param);
> +
> +    let chunk_path = Path::new(&chunk);
> +    let digest_str = chunk_path.file_name().unwrap().to_str().unwrap();
> +    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("-")),
> +    );
> +
> +    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)?;
> +
> +    let mut referenced_by = None;
> +    if let Some(search_path) = search_path {
> +        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();
> +            let mut index: Option<Box<dyn IndexFile>> = None;
> +
> +            if file_name.ends_with(".fidx") {
> +                index = match FixedIndexReader::open(entry.path()) {
> +                    Ok(index) => Some(Box::new(index)),
> +                    Err(_) => None,
> +                };
> +            }
> +
> +            if file_name.ends_with(".didx") {
> +                index = match DynamicIndexReader::open(entry.path()) {
> +                    Ok(index) => Some(Box::new(index)),
> +                    Err(_) => None,
> +                };
> +            }
> +
> +            if let Some(index) = index {
> +                for pos in 0..index.index_count() {
> +                    if let Some(index_chunk_digest) = index.index_digest(pos) {
> +                        let str = proxmox::tools::digest_to_hex(index_chunk_digest);
> +                        if str.eq(digest_str) {

why do you compare str here?
would it not be enough to compare index_chunk_digest to digest_raw ?
i guess a comparison of to fixed sized arrays is faster than comparing 
to str ? it's at least a comparison less, and if i let this run over
my whole datastore this can easily add up

> +                            references.push(entry.path().to_string_lossy().into_owned());
> +                            break;
> +                        }
> +                    }
> +                }
> +            }
> +        }
> +        referenced_by = Some(references);
> +    }
> +
> +    if decode_output_path.is_some() || to_stdout {
> +        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));
> +
> +    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 cc782da2..2ab8157e 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;
>   
> @@ -566,3 +566,12 @@ pub fn create_run_dir() -> Result<(), Error> {
>       let _: bool = proxmox::tools::fs::create_path(PROXMOX_BACKUP_RUN_DIR_M!(), None, None)?;
>       Ok(())
>   }
> +
> +/// Writes data to stdout or a file, based on whether a path is given.
> +pub fn output_or_stdout<P: AsRef<Path>>(path: Option<P>, data: &[u8]) -> Result<(), Error> {
> +    if let Some(path) = path {
> +        File::create(path)?.write_all(data).map_err(Error::new)
> +    } else {
> +        stdout().write_all(data).map_err(Error::new)
> +    }
> +}
>