Re: [pbs-devel] [RFC PATCH proxmox-backup 1/5] api2/admin/datastore: refactor list_dir_content in catalog_reader

[Dietmar Maurer <dietmar@proxmox.com>]

comments inline

> On 12/21/2020 12:25 PM Dominik Csapak <d.csapak@proxmox.com> wrote:
> 
>  
> we will reuse that later in the client, so we need it somewhere
> we can use from there
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/api2/admin/datastore.rs | 52 ++++-------------------------
>  src/backup/catalog.rs       | 65 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+), 45 deletions(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 16fee943..5f06a2bf 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -1302,7 +1302,7 @@ fn catalog(
>      _param: Value,
>      _info: &ApiMethod,
>      rpcenv: &mut dyn RpcEnvironment,
> -) -> Result<Value, Error> {
> +) -> Result<Vec<Value>, Error> {
>      let datastore = DataStore::lookup_datastore(&store)?;
>  
>      let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> @@ -1334,52 +1334,14 @@ fn catalog(
>      let reader = BufferedDynamicReader::new(index, chunk_reader);
>  
>      let mut catalog_reader = CatalogReader::new(reader);
> -    let mut current = catalog_reader.root()?;
> -    let mut components = vec![];
> -
>  
> -    if filepath != "root" {
> -        components = base64::decode(filepath)?;
> -        if components.len() > 0 && components[0] == '/' as u8 {
> -            components.remove(0);
> -        }
> -        for component in components.split(|c| *c == '/' as u8) {
> -            if let Some(entry) = catalog_reader.lookup(&current, component)? {
> -                current = entry;
> -            } else {
> -                bail!("path {:?} not found in catalog", &String::from_utf8_lossy(&components));
> -            }
> -        }
> -    }
> -
> -    let mut res = Vec::new();
> -
> -    for direntry in catalog_reader.read_dir(&current)? {
> -        let mut components = components.clone();
> -        components.push('/' as u8);
> -        components.extend(&direntry.name);
> -        let path = base64::encode(components);
> -        let text = String::from_utf8_lossy(&direntry.name);
> -        let mut entry = json!({
> -            "filepath": path,
> -            "text": text,
> -            "type": CatalogEntryType::from(&direntry.attr).to_string(),
> -            "leaf": true,
> -        });
> -        match direntry.attr {
> -            DirEntryAttribute::Directory { start: _ } => {
> -                entry["leaf"] = false.into();
> -            },
> -            DirEntryAttribute::File { size, mtime } => {
> -                entry["size"] = size.into();
> -                entry["mtime"] = mtime.into();
> -            },
> -            _ => {},
> -        }
> -        res.push(entry);
> -    }
> +    let path = if filepath != "root" {
> +        base64::decode(filepath)?
> +    } else {
> +        vec![b'/']
> +    };
>  
> -    Ok(res.into())
> +    catalog_reader.list_dir_content(&path)
>  }
>  
>  fn recurse_files<'a, T, W>(
> diff --git a/src/backup/catalog.rs b/src/backup/catalog.rs
> index b500fb93..5f8e85a6 100644
> --- a/src/backup/catalog.rs
> +++ b/src/backup/catalog.rs
> @@ -5,6 +5,7 @@ use std::io::{Read, Write, Seek, SeekFrom};
>  use std::os::unix::ffi::OsStrExt;
>  
>  use anyhow::{bail, format_err, Error};
> +use serde_json::{json, Value};
>  
>  use pathpatterns::{MatchList, MatchType};
>  use proxmox::tools::io::ReadExt;
> @@ -474,6 +475,32 @@ impl <R: Read + Seek> CatalogReader<R> {
>          Ok(entry_list)
>      }
>  
> +    /// Lookup a DirEntry from an absolute path
> +    pub fn lookup_recursive(
> +        &mut self,
> +        path: &[u8],
> +    ) -> Result<DirEntry, Error> {
> +        let mut current = self.root()?;
> +        if path == b"/" {
> +            return Ok(current);
> +        }
> +
> +        let components = if path.len() > 0 && path[0] == b'/' {
> +            &path[1..]
> +        } else {
> +            path
> +        }.split(|c| *c == b'/');
> +
> +        for comp in components {
> +            if let Some(entry) = self.lookup(&current, comp)? {
> +                current = entry;
> +            } else {
> +                bail!("path {:?} not found in catalog", String::from_utf8_lossy(&path));
> +            }
> +        }
> +        Ok(current)
> +    }
> +

This is OK for me

>      /// Lockup a DirEntry inside a parent directory
>      pub fn lookup(
>          &mut self,
> @@ -554,6 +581,44 @@ impl <R: Read + Seek> CatalogReader<R> {
>          })
>      }
>  
> +    /// Returns the list of content of the given path as json
> +    pub fn list_dir_content(&mut self, path: &[u8]) -> Result<Vec<Value>, Error> {
> +        let dir = self.lookup_recursive(path)?;
> +        let mut res = vec![];
> +        let mut path = path.to_vec();
> +        if path.len() > 0 && path[0] == b'/' {
> +            path.remove(0);
> +        }
> +
> +        for direntry in self.read_dir(&dir)? {
> +            let mut components = path.clone();
> +            components.push(b'/');
> +            components.extend(&direntry.name);
> +            let path = base64::encode(&components);
> +            let text = String::from_utf8_lossy(&direntry.name);
> +            let mut entry = json!({
> +                "filepath": path,
> +                "text": text,
> +                "name": direntry.name.clone(),
> +                "type": CatalogEntryType::from(&direntry.attr).to_string(),
> +                "leaf": true,
> +            });
> +            match direntry.attr {
> +                DirEntryAttribute::Directory { start: _ } => {
> +                    entry["leaf"] = false.into();
> +                },
> +                DirEntryAttribute::File { size, mtime } => {
> +                    entry["size"] = size.into();
> +                    entry["mtime"] = mtime.into();
> +                },
> +                _ => {},
> +            }
> +            res.push(entry);
> +        }

But this is API code, so we should find another place for this.

> +
> +        Ok(res)
> +    }
> +
>      /// Finds all entries matching the given match patterns and calls the
>      /// provided callback on them.
>      pub fn find(
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel