Re: [pbs-devel] [PATCH v5 proxmox-backup 4/4] add index recovery to pb-debug

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

On Thu, Apr 29, 2021 at 01:00:16PM +0200, Hannes Laimer wrote:
> Adds possibility to recover data from an index file. Options:
>  - chunks: path to the directory where the chunks are saved
>  - file: the index file that should be recovered(must be either .fidx or
>    didx)
>  - [opt] keyfile: path to a keyfile, if the data was encrypted, a keyfile is
>    needed
>  - [opt] skip-crc: boolean, if true, read chunks wont be verified with their
>    crc-sum, increases the restore speed by a lot
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> v5:
>  - try to load decryption key at the beginning
>  - simplified usage of the key
>  - allocate 4 MiB for chunks instead of 1 MiB
> 
> v4:
>  - there might be a better way to do the match magic block
> 
>  src/bin/proxmox-backup-debug.rs         |   6 +-
>  src/bin/proxmox_backup_debug/mod.rs     |   2 +
>  src/bin/proxmox_backup_debug/recover.rs | 115 ++++++++++++++++++++++++
>  3 files changed, 121 insertions(+), 2 deletions(-)
>  create mode 100644 src/bin/proxmox_backup_debug/recover.rs
> 
> diff --git a/src/bin/proxmox-backup-debug.rs b/src/bin/proxmox-backup-debug.rs
> index fb548a31..83328701 100644
> --- a/src/bin/proxmox-backup-debug.rs
> +++ b/src/bin/proxmox-backup-debug.rs
> @@ -2,7 +2,7 @@ use proxmox::api::cli::*;
>  use proxmox::api::schema::{Schema, StringSchema};
>  
>  mod proxmox_backup_debug;
> -use proxmox_backup_debug::inspect_commands;
> +use proxmox_backup_debug::{inspect_commands, recover_commands};
>  
>  pub mod proxmox_client_tools;
>  use proxmox_client_tools::key_source::{get_encryption_key_password, KEYFILE_SCHEMA};
> @@ -12,7 +12,9 @@ pub const PATH_SCHEMA: Schema = StringSchema::new("Path to a file or a directory
>  fn main() {
>      proxmox_backup::tools::setup_safe_path_env();
>  
> -    let cmd_def = CliCommandMap::new().insert("inspect", inspect_commands());
> +    let cmd_def = CliCommandMap::new()
> +        .insert("inspect", inspect_commands())
> +        .insert("recover", recover_commands());
>  
>      let rpcenv = CliEnvironment::new();
>      run_cli_command(
> diff --git a/src/bin/proxmox_backup_debug/mod.rs b/src/bin/proxmox_backup_debug/mod.rs
> index 644583db..62df7754 100644
> --- a/src/bin/proxmox_backup_debug/mod.rs
> +++ b/src/bin/proxmox_backup_debug/mod.rs
> @@ -1,2 +1,4 @@
>  mod inspect;
>  pub use inspect::*;
> +mod recover;
> +pub use recover::*;
> diff --git a/src/bin/proxmox_backup_debug/recover.rs b/src/bin/proxmox_backup_debug/recover.rs
> new file mode 100644
> index 00000000..2355b50f
> --- /dev/null
> +++ b/src/bin/proxmox_backup_debug/recover.rs
> @@ -0,0 +1,115 @@
> +use std::fs::File;
> +use std::io::{Read, Seek, SeekFrom, Write};
> +use std::path::Path;
> +
> +use anyhow::{format_err, Error};
> +
> +use proxmox::api::api;
> +use proxmox::api::cli::{CliCommand, CliCommandMap, CommandLineInterface};
> +use proxmox_backup::backup::{DYNAMIC_SIZED_CHUNK_INDEX_1_0, FIXED_SIZED_CHUNK_INDEX_1_0};
> +use serde_json::Value;
> +
> +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::tools::digest_to_hex;
> +
> +#[api(
> +    input: {
> +        properties: {
> +            file: {
> +                schema: PATH_SCHEMA,

Actually, since this schema has no restrictions, all this does is give
the same description to all parameters using it. So please, replace
`schema: PATH_SCHEMA` with a `description: "Something more useful"` ;-)

Here, below, and the other commands as well.

It's particularly weird to see *this* command's `--help` output since it
has 2 of them:

    $ proxmox-backup-debug recover index --help
    <snip>

    Usage: proxmox-backup-debug recover index --chunks <string> --file <string> [OPTIONS]
     --chunks   <string>
                 Path to a file or a directory

     --file     <string>
                 Path to a file or a directory

> +            },
> +            chunks: {
> +                schema: PATH_SCHEMA,
> +            },
> +            "keyfile": {
> +                schema: KEYFILE_SCHEMA,
> +                optional: true,
> +            },
> +            "skip-crc": {
> +                type: Boolean,
> +                optional: true,
> +                default: false,
> +                description: "Skip the crc verification, increases the restore speed immensely",
> +            }
> +        }
> +    }
> +)]
> +/// Restore the data from an index file, given the directory of where chunks
> +/// are saved, the index file and a keyfile, if needed for decryption.
> +fn recover_index(
> +    file: String,
> +    chunks: String,
> +    keyfile: Option<String>,
> +    skip_crc: bool,
> +    _param: Value,
> +) -> Result<(), Error> {
> +    let file_path = Path::new(&file);
> +    let chunks_path = Path::new(&chunks);
> +
> +    let key_file_path = keyfile.as_ref().map(Path::new);
> +
> +    let mut file = File::open(Path::new(&file))?;
> +    let mut magic = [0; 8];
> +    file.read_exact(&mut magic)?;
> +    file.seek(SeekFrom::Start(0))?;
> +    let index: Box<dyn IndexFile> = match magic {
> +        FIXED_SIZED_CHUNK_INDEX_1_0 => {
> +            Ok(Box::new(FixedIndexReader::new(file)?) as Box<dyn IndexFile>)

Please drop the `Ok()` wrapping here

> +        }
> +        DYNAMIC_SIZED_CHUNK_INDEX_1_0 => {
> +            Ok(Box::new(DynamicIndexReader::new(file)?) as Box<dyn IndexFile>)

and here

> +        }
> +        _ => Err(format_err!(

and use bail!() here instead of `Err(format_err!(...))`

> +            "index file must either be a .fidx or a .didx file"
> +        )),
> +    }?;

and drop the `?` here.

> +
> +    let mut crypt_conf_opt = None;
> +
> +    if key_file_path.is_some() {

Also please combine the above 2 lines to:
    let crypt_conf_opt = if key_file_path.is_some() {
        ...
    } else {
        None
    };

Btw. another variant with short optional things would be:

    let crypt_conf_opt = key_file_path
        .map(|key_file_path| {
            let (key, _created, _fingerprint) =
                load_and_decrypt_key(key_file_path, &get_encryption_key_password)?;
            CryptConfig::new(key)
        })
        .transpose()?;

^ The `transpose` helps bubbling up the error.


> +        let (key, _created, _fingerprint) =
> +            load_and_decrypt_key(&key_file_path.unwrap(), &get_encryption_key_password)?;
> +        crypt_conf_opt = Some(CryptConfig::new(key)?);
> +    }
> +
> +    let output_filename = file_path.file_stem().unwrap().to_str().unwrap();
> +    let output_path = Path::new(output_filename);
> +    let mut output_file = File::create(output_path)
> +        .map_err(|e| format_err!("could not create output file - {}", e))?;
> +
> +    let mut data = Vec::with_capacity(4 * 1024 * 1024);
> +    for pos in 0..index.index_count() {
> +        let chunk_digest = index.index_digest(pos).unwrap();
> +        let digest_str = digest_to_hex(chunk_digest);
> +        let digest_prefix = &digest_str[0..4];
> +        let chunk_path = chunks_path.join(digest_prefix).join(digest_str);
> +        let mut chunk_file = std::fs::File::open(&chunk_path)
> +            .map_err(|e| format_err!("could not open chunk file - {}", e))?;
> +
> +        data.clear();
> +        chunk_file.read_to_end(&mut data)?;
> +        let chunk_blob = DataBlob::from_raw(data.clone())?;
> +
> +        if !skip_crc {
> +            chunk_blob.verify_crc()?;
> +        }
> +
> +        output_file.write_all(
> +            chunk_blob
> +                .decode(crypt_conf_opt.as_ref(), Some(chunk_digest))?
> +                .as_slice(),
> +        )?;
> +    }
> +
> +    Ok(())
> +}
> +
> +pub fn recover_commands() -> CommandLineInterface {
> +    let cmd_def = CliCommandMap::new().insert("index", CliCommand::new(&API_METHOD_RECOVER_INDEX));
> +    cmd_def.into()
> +}
> -- 
> 2.20.1