Re: [pbs-devel] [PATCH v5 proxmox-backup 3/4] add file inspection to pb-debug

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Thu, Apr 29, 2021 at 01:00:15PM +0200, Hannes Laimer wrote:
> Adds possibility to inspect .blob, .fidx and .didx files. For index
> files a list of the chunks referenced will be printed in addition to
> some other inforation. .blob files can be decoded into file or directly
> into stdout. Options:
>  - file: path to the file
>  - [opt] decode: path to a file or stdout(-), if specidied, the file will be
>    decoded into the specified location [only for blob files, no effect
>   with index files]
>  - [opt] keyfile: path to a keyfile, needed if decode is specified and the
>    data was encrypted
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> v5:
>  - combine path for fixed and dynamic index fliles to avoid duplicate
>    code
> 
> v4:
>  - only the types of file that are passed by the user are check with the
>    magic number, when looking for index files just the filename ending
>    is checked -> don't have to open the file for that
>  - not sure if a function for the magic nr reading, seek reset makes
>    sense(?), it's just two lines
>  
>  src/bin/proxmox_backup_debug/inspect.rs | 119 ++++++++++++++++++++++--
>  1 file changed, 112 insertions(+), 7 deletions(-)
> 
> diff --git a/src/bin/proxmox_backup_debug/inspect.rs b/src/bin/proxmox_backup_debug/inspect.rs
> index dd6ee287..2d95448e 100644
> --- a/src/bin/proxmox_backup_debug/inspect.rs
> +++ b/src/bin/proxmox_backup_debug/inspect.rs
> @@ -1,19 +1,26 @@
> +use std::collections::HashSet;
> +use std::fs::File;
> +use std::io::{Read, Seek, SeekFrom};
>  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 crate::{get_encryption_key_password, KEYFILE_SCHEMA, PATH_SCHEMA};
> +use proxmox::api::{
> +    api,
> +    cli::{
> +        format_and_print_result, get_output_format, CliCommand, CliCommandMap,
> +        CommandLineInterface, OUTPUT_FORMAT,
> +    },
> +};
>  use proxmox_backup::backup::{
>      load_and_decrypt_key, CryptConfig, DataBlob, DynamicIndexReader, FixedIndexReader, IndexFile,
> +    COMPRESSED_BLOB_MAGIC_1_0, DYNAMIC_SIZED_CHUNK_INDEX_1_0, ENCRYPTED_BLOB_MAGIC_1_0,
> +    ENCR_COMPR_BLOB_MAGIC_1_0, FIXED_SIZED_CHUNK_INDEX_1_0, UNCOMPRESSED_BLOB_MAGIC_1_0,
>  };
>  
> -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
> @@ -37,6 +44,102 @@ fn decode_blob(
>      Ok(())
>  }
>  
> +#[api(
> +    input: {
> +        properties: {
> +            file: {
> +                schema: PATH_SCHEMA,
> +            },
> +            "decode": {
> +                schema: PATH_SCHEMA,
> +                optional: true,
> +            },
> +            "keyfile": {
> +                schema: KEYFILE_SCHEMA,
> +                optional: true,
> +            },
> +            "output-format": {
> +                schema: OUTPUT_FORMAT,
> +                optional: true,
> +            },
> +        }
> +    }
> +)]
> +/// Inspect a file
> +fn inspect_file(
> +    file: String,
> +    decode: Option<String>,
> +    keyfile: Option<String>,
> +    param: Value,
> +) -> Result<(), Error> {
> +    let output_format = get_output_format(&param);
> +
> +    let mut file = File::open(Path::new(&file))?;
> +    let mut magic = [0; 8];
> +    file.read_exact(&mut magic)?;
> +    file.seek(SeekFrom::Start(0))?;
> +    let val = match magic {
> +        UNCOMPRESSED_BLOB_MAGIC_1_0
> +        | COMPRESSED_BLOB_MAGIC_1_0
> +        | ENCRYPTED_BLOB_MAGIC_1_0
> +        | ENCR_COMPR_BLOB_MAGIC_1_0 => {
> +            let data_blob = DataBlob::load_from_reader(&mut file)?;
> +            let key_file_path = keyfile.as_ref().map(Path::new);
> +
> +            let (decode_output_path, to_stdout) = (

Same reasoning about `to_stdout` here as in the other patch. I think it
can just be dropped.

> +                decode.as_ref().map(Path::new),
> +                decode.clone().map_or(false, |p| p.eq("-")),
> +            );
> +
> +            if decode_output_path.is_some() || to_stdout {
> +                decode_blob(decode_output_path, key_file_path, None, &data_blob)?;
> +            }
> +
> +            let crypt_mode = data_blob.crypt_mode()?;
> +            Ok(json!({

Please remove the `Ok` wrapping here.

> +                "encryption": crypt_mode,
> +                "raw_size": data_blob.raw_size(),
> +            }))
> +        }
> +        FIXED_SIZED_CHUNK_INDEX_1_0 | DYNAMIC_SIZED_CHUNK_INDEX_1_0 => {
> +            let index: Box<dyn IndexFile> = match magic {
> +                FIXED_SIZED_CHUNK_INDEX_1_0 => {
> +                    Ok(Box::new(FixedIndexReader::new(file)?) as Box<dyn IndexFile>)
> +                }
> +                DYNAMIC_SIZED_CHUNK_INDEX_1_0 => {
> +                    Ok(Box::new(DynamicIndexReader::new(file)?) as Box<dyn IndexFile>)
> +                }
> +                _ => Err(format_err!("This is technically not possible")),
> +            }?;
> +
> +            let mut ctime_str = index.index_ctime().to_string();
> +            if let Ok(s) = proxmox::tools::time::strftime_local("%c", index.index_ctime()) {
> +                ctime_str = s;
> +            }
> +
> +            let mut chunk_digests = HashSet::new();
> +
> +            for pos in 0..index.index_count() {
> +                let digest = index.index_digest(pos).unwrap();
> +                chunk_digests.insert(proxmox::tools::digest_to_hex(digest));
> +            }
> +
> +            Ok(json!({

and here

> +                "size": index.index_size(),
> +                "ctime": ctime_str,
> +                "chunk-digests": chunk_digests
> +            }))
> +        }
> +        _ => Err(format_err!(

and use `bail!` instead of `Err(format_err!(` followed by a `?` below.

> +            "Only .blob, .fidx and .didx files may be inspected"
> +        )),
> +    }?;

^ And just drop the `?` here.

> +
> +    format_and_print_result(&val, &output_format);
> +
> +    Ok(())
> +}
> +
>  #[api(
>      input: {
>          properties: {
> @@ -156,7 +259,9 @@ fn inspect_chunk(
>  }
>  
>  pub fn inspect_commands() -> CommandLineInterface {
> -    let cmd_def = CliCommandMap::new().insert("chunk", CliCommand::new(&API_METHOD_INSPECT_CHUNK));
> +    let cmd_def = CliCommandMap::new()
> +        .insert("file", CliCommand::new(&API_METHOD_INSPECT_FILE))

Here too, I think we should add:

    .arg_param(["file"])

> +        .insert("chunk", CliCommand::new(&API_METHOD_INSPECT_CHUNK));
>  
>      cmd_def.into()
>  }
> -- 
> 2.20.1