[pbs-devel] [PATCH v3 proxmox-backup 0/4] debug cli: improve output, optionally compare file content for `diff archive`

[pbs-devel] [PATCH v3 proxmox-backup 0/4] debug cli: improve output, optionally compare file content for `diff archive`

[Lukas Wagner]

This patch series contains a few improvements for the `diff archive` tool,
mainly based on Wolfgang's and Thomas' suggestions.

First, the output of is now much more detailed and shows
some relevant file attributes, including what has changed between
snapshots. Changed attributes are highlighted by a "*" and are
highlighted by colored text.

For instance:

$ proxmox-backup-debug diff archive ... --compare-content
A  f   644  10045  10000     0 B  2022-11-28 13:44:51  add.txt
M  f   644  10045  10000     6 B *2022-11-28 13:45:05 *content.txt
D  f   644  10045  10000     0 B  2022-11-28 13:17:09  deleted.txt
M  f   644  10045    *29     0 B  2022-11-28 13:16:20  gid.txt
M  f  *777  10045  10000     0 B  2022-11-28 13:42:47  mode.txt
M  f   644  10045  10000     0 B *2022-11-28 13:44:33  mtime.txt
M  f   644  10045  10000    *7 B *2022-11-28 13:44:59  size.txt
M  f   644 *64045  10000     0 B  2022-11-28 13:16:18  uid.txt
M *f   644  10045  10000    10 B  2022-11-28 13:44:59  type.txt

Furthermore, there now exists the possiblity to pass
the --compare-content flag to the tool. If the flag is passed,
the tool will compare the file content instead of relying on mtime
alone to detect modifications.

This patch series adds new dependencies to the `termcolor` and `atty`
crates. That being said, both crates were already pulled in
by other crates as transitive dependencies.

Changes from v2:
  - Increase buffersize to 4K
  - Added workaround for weird issue with tokio::try_join! and #[api]
  - Drop dependency on `atty` crate
  - Remove modificiations to debian/control
  - `diff_archive_cmd`: Moved parameters into the function signature,
    instead of manually extracting it from `Value`.


Changes from v1:
  - Made `changed indicator for file content` a bit less confusing.
    For regular files, it is now only displayed if
      - the --comapare-content flag is set and
      - the file contents *actually* differ

  - Removed unnecessesary namespace prefix for std::task::Pin in unit tests
  - Added color output, controllable via --color {always,auto,never} flag.


Lukas Wagner (4):
  debug cli: show more file attributes for `diff archive` command
  debug cli: add 'compare-content' flag to `diff archive` command
  debug cli: add colored output for `diff archive`
  debug cli: move parameters into the function signature

 Cargo.toml                             |   1 +
 src/bin/proxmox_backup_debug/diff.rs   | 611 ++++++++++++++++++++++---
 src/bin/proxmox_backup_manager/ldap.rs | 102 +++++
 3 files changed, 644 insertions(+), 70 deletions(-)
 create mode 100644 src/bin/proxmox_backup_manager/ldap.rs

-- 
2.30.2

[pbs-devel] [PATCH v3 proxmox-backup 1/4] debug cli: show more file attributes for `diff archive` command

[Lukas Wagner]

This commit enriches the output of the `diff archive` command,
showing pxar entry type, mode, uid, gid, size, mtime and filename.
Attributes that changed between both snapshots are prefixed
with a "*".

For instance:

$ proxmox-backup-debug diff archive ...
A  f   644  10045  10000    0 B  2022-11-28 13:44:51  add.txt
M  f   644  10045  10000    6 B *2022-11-28 13:45:05  content.txt
D  f   644  10045  10000    0 B  2022-11-28 13:17:09  deleted.txt
M  f   644  10045    *29    0 B  2022-11-28 13:16:20  gid.txt
M  f  *777  10045  10000    0 B  2022-11-28 13:42:47  mode.txt
M  f   644  10045  10000    0 B *2022-11-28 13:44:33  mtime.txt
M  f   644  10045  10000   *7 B *2022-11-28 13:44:59 *size.txt
M  f   644 *64045  10000    0 B  2022-11-28 13:16:18  uid.txt
M *f   644  10045  10000   10 B  2022-11-28 13:44:59  type_changed.txt

Also, this commit ensures that we always show the *new* type.
Previously, the command showed the old type if it was changed.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/bin/proxmox_backup_debug/diff.rs | 221 ++++++++++++++++++++-------
 1 file changed, 167 insertions(+), 54 deletions(-)

diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
index cb157ec2..d5a4a1fe 100644
--- a/src/bin/proxmox_backup_debug/diff.rs
+++ b/src/bin/proxmox_backup_debug/diff.rs
@@ -11,7 +11,7 @@ use futures::FutureExt;
 use proxmox_router::cli::{CliCommand, CliCommandMap, CommandLineInterface};
 use proxmox_schema::api;
 
-use pbs_api_types::{BackupNamespace, BackupPart};
+use pbs_api_types::{BackupNamespace, BackupPart, HumanByte};
 use pbs_client::tools::key_source::{
     crypto_parameters, format_key_source, get_encryption_key_password, KEYFD_SCHEMA,
 };
@@ -173,15 +173,18 @@ async fn diff_archive(
     // If file is present in both snapshots, it *might* be modified, but does not have to be.
     // If another, unmodified file resides in the same chunk as an actually modified one,
     // it will also show up as modified here...
-    let potentially_modified: HashMap<&OsStr, &FileEntry> = files_in_a
+    let potentially_modified: HashMap<&OsStr, (&FileEntry, &FileEntry)> = files_in_a
         .iter()
-        .filter(|(path, _)| files_in_b.contains_key(*path))
-        .map(|(path, entry)| (path.as_os_str(), entry))
+        .filter_map(|(path, entry_a)| {
+            files_in_b
+                .get(path)
+                .map(|entry_b| (path.as_os_str(), (entry_a, entry_b)))
+        })
         .collect();
 
     // ... so we compare the file metadata/contents to narrow the selection down to files
     // which where *really* modified.
-    let modified_files = compare_files(&files_in_a, &files_in_b, potentially_modified).await?;
+    let modified_files = compare_files(potentially_modified).await?;
 
     show_file_list(&added_files, &deleted_files, &modified_files);
 
@@ -335,7 +338,6 @@ fn visit_directory<'f, 'c>(
                 let digest = chunk_list.get(chunk_index).context("Invalid chunk index")?;
 
                 if chunk_diff.get(digest).is_some() {
-                    // files.insert(file_path.clone().into_os_string());
                     entries.insert(file_path.into_os_string(), entry);
                     break;
                 }
@@ -349,62 +351,177 @@ fn visit_directory<'f, 'c>(
 
 /// Check if files were actually modified
 async fn compare_files<'a>(
-    entries_a: &HashMap<OsString, FileEntry>,
-    entries_b: &HashMap<OsString, FileEntry>,
-    files: HashMap<&'a OsStr, &'a FileEntry>,
-) -> Result<HashMap<&'a OsStr, &'a FileEntry>, Error> {
+    files: HashMap<&'a OsStr, (&'a FileEntry, &'a FileEntry)>,
+) -> Result<HashMap<&'a OsStr, (&'a FileEntry, ChangedProperties)>, Error> {
     let mut modified_files = HashMap::new();
 
-    for (path, entry) in files {
-        let p = path.to_os_string();
-        let file_a = entries_a.get(&p).context("File entry not in map")?;
-        let file_b = entries_b.get(&p).context("File entry not in map")?;
-
-        if !compare_file(file_a, file_b).await {
-            modified_files.insert(path, entry);
+    for (path, (entry_a, entry_b)) in files {
+        if let Some(changed) = compare_file(entry_a, entry_b).await? {
+            modified_files.insert(path, (entry_b, changed));
         }
     }
 
     Ok(modified_files)
 }
 
-async fn compare_file(file_a: &FileEntry, file_b: &FileEntry) -> bool {
-    if file_a.metadata() != file_b.metadata() {
-        // Check if mtime, permissions, ACLs, etc. have changed - if they have changed, we consider
-        // the file as modified.
-        return false;
-    }
+async fn compare_file(
+    file_a: &FileEntry,
+    file_b: &FileEntry,
+) -> Result<Option<ChangedProperties>, Error> {
+    let mut changed = ChangedProperties::default();
+
+    changed.set_from_metadata(file_a, file_b);
 
     match (file_a.kind(), file_b.kind()) {
         (EntryKind::Symlink(a), EntryKind::Symlink(b)) => {
             // Check whether the link target has changed.
-            a.as_os_str() == b.as_os_str()
+            changed.content = a.as_os_str() != b.as_os_str();
         }
         (EntryKind::Hardlink(a), EntryKind::Hardlink(b)) => {
             // Check whether the link target has changed.
-            a.as_os_str() == b.as_os_str()
+            changed.content = a.as_os_str() != b.as_os_str();
+        }
+        (EntryKind::Device(a), EntryKind::Device(b)) => {
+            changed.content = a.major != b.major || a.minor != b.minor
         }
-        (EntryKind::Device(a), EntryKind::Device(b)) => a.major == b.major && a.minor == b.minor,
-        (EntryKind::Socket, EntryKind::Socket) => true,
-        (EntryKind::Fifo, EntryKind::Fifo) => true,
         (EntryKind::File { size: size_a, .. }, EntryKind::File { size: size_b, .. }) => {
-            // At this point we know that all metadata including mtime is
-            // the same. To speed things up, we consider the files as equal if they also have
-            // the same size.
-            // If one were completely paranoid, one could compare the actual file contents,
-            // but this decreases performance drastically.
-            size_a == size_b
+            if size_a != size_b {
+                changed.size = true;
+                changed.content = true;
+            };
+        }
+        (EntryKind::Directory, EntryKind::Directory) => {}
+        (EntryKind::Socket, EntryKind::Socket) => {}
+        (EntryKind::Fifo, EntryKind::Fifo) => {}
+        (_, _) => {
+            changed.entry_type = true;
         }
-        (EntryKind::Directory, EntryKind::Directory) => true,
-        (_, _) => false, // Kind has changed, so we of course consider it modified.
+    }
+
+    if changed.any() {
+        Ok(Some(changed))
+    } else {
+        Ok(None)
+    }
+}
+
+#[derive(Copy, Clone, Default)]
+struct ChangedProperties {
+    entry_type: bool,
+    mtime: bool,
+    acl: bool,
+    xattrs: bool,
+    fcaps: bool,
+    quota_project_id: bool,
+    mode: bool,
+    flags: bool,
+    uid: bool,
+    gid: bool,
+    size: bool,
+    content: bool,
+}
+
+impl ChangedProperties {
+    fn set_from_metadata(&mut self, file_a: &FileEntry, file_b: &FileEntry) {
+        let a = file_a.metadata();
+        let b = file_b.metadata();
+
+        self.acl = a.acl != b.acl;
+        self.xattrs = a.xattrs != b.xattrs;
+        self.fcaps = a.fcaps != b.fcaps;
+        self.quota_project_id = a.quota_project_id != b.quota_project_id;
+        self.mode = a.stat.mode != b.stat.mode;
+        self.flags = a.stat.flags != b.stat.flags;
+        self.uid = a.stat.uid != b.stat.uid;
+        self.gid = a.stat.gid != b.stat.gid;
+        self.mtime = a.stat.mtime != b.stat.mtime;
+    }
+
+    fn any(&self) -> bool {
+        self.entry_type
+            || self.mtime
+            || self.acl
+            || self.xattrs
+            || self.fcaps
+            || self.quota_project_id
+            || self.mode
+            || self.flags
+            || self.uid
+            || self.gid
+            || self.content
+    }
+}
+
+fn change_indicator(changed: bool) -> &'static str {
+    if changed {
+        "*"
+    } else {
+        " "
+    }
+}
+
+fn format_filesize(entry: &FileEntry, changed: bool) -> String {
+    if let Some(size) = entry.file_size() {
+        format!(
+            "{}{:.1}",
+            change_indicator(changed),
+            HumanByte::new_decimal(size as f64)
+        )
+    } else {
+        String::new()
     }
 }
 
+fn format_mtime(entry: &FileEntry, changed: bool) -> String {
+    let mtime = &entry.metadata().stat.mtime;
+
+    let format = if changed { "*%F %T" } else { " %F %T" };
+
+    proxmox_time::strftime_local(format, mtime.secs).unwrap_or_default()
+}
+
+fn format_mode(entry: &FileEntry, changed: bool) -> String {
+    let mode = entry.metadata().stat.mode & 0o7777;
+    format!("{}{:o}", change_indicator(changed), mode)
+}
+
+fn format_entry_type(entry: &FileEntry, changed: bool) -> String {
+    let kind = match entry.kind() {
+        EntryKind::Symlink(_) => "l",
+        EntryKind::Hardlink(_) => "h",
+        EntryKind::Device(_) if entry.metadata().stat.is_blockdev() => "b",
+        EntryKind::Device(_) => "c",
+        EntryKind::Socket => "s",
+        EntryKind::Fifo => "p",
+        EntryKind::File { .. } => "f",
+        EntryKind::Directory => "d",
+        _ => " ",
+    };
+
+    format!("{}{}", change_indicator(changed), kind)
+}
+
+fn format_uid(entry: &FileEntry, changed: bool) -> String {
+    format!("{}{}", change_indicator(changed), entry.metadata().stat.uid)
+}
+
+fn format_gid(entry: &FileEntry, changed: bool) -> String {
+    format!("{}{}", change_indicator(changed), entry.metadata().stat.gid)
+}
+
+fn format_file_name(entry: &FileEntry, changed: bool) -> String {
+    format!(
+        "{}{}",
+        change_indicator(changed),
+        entry.file_name().to_string_lossy()
+    )
+}
+
 /// Display a sorted list of added, modified, deleted files.
 fn show_file_list(
     added: &HashMap<&OsStr, &FileEntry>,
     deleted: &HashMap<&OsStr, &FileEntry>,
-    modified: &HashMap<&OsStr, &FileEntry>,
+    modified: &HashMap<&OsStr, (&FileEntry, ChangedProperties)>,
 ) {
     let mut all: Vec<&OsStr> = Vec::new();
 
@@ -415,28 +532,24 @@ fn show_file_list(
     all.sort();
 
     for file in all {
-        let (op, entry) = if let Some(entry) = added.get(file) {
-            ("A", *entry)
+        let (op, entry, changed) = if let Some(entry) = added.get(file) {
+            ("A", entry, ChangedProperties::default())
         } else if let Some(entry) = deleted.get(file) {
-            ("D", *entry)
-        } else if let Some(entry) = modified.get(file) {
-            ("M", *entry)
+            ("D", entry, ChangedProperties::default())
+        } else if let Some((entry, changed)) = modified.get(file) {
+            ("M", entry, *changed)
         } else {
             unreachable!();
         };
 
-        let entry_kind = match entry.kind() {
-            EntryKind::Symlink(_) => "l",
-            EntryKind::Hardlink(_) => "h",
-            EntryKind::Device(_) if entry.metadata().stat.is_blockdev() => "b",
-            EntryKind::Device(_) => "c",
-            EntryKind::Socket => "s",
-            EntryKind::Fifo => "p",
-            EntryKind::File { .. } => "f",
-            EntryKind::Directory => "d",
-            _ => " ",
-        };
+        let entry_type = format_entry_type(entry, changed.entry_type);
+        let uid = format_uid(entry, changed.uid);
+        let gid = format_gid(entry, changed.gid);
+        let mode = format_mode(entry, changed.mode);
+        let size = format_filesize(entry, changed.size);
+        let mtime = format_mtime(entry, changed.mtime);
+        let name = format_file_name(entry, changed.content);
 
-        println!("{} {} {}", op, entry_kind, file.to_string_lossy());
+        println!("{op} {entry_type:>2} {mode:>5} {uid:>6} {gid:>6} {size:>10} {mtime:11} {name}");
     }
 }
-- 
2.30.2

[pbs-devel] [PATCH v3 proxmox-backup 2/4] debug cli: add 'compare-content' flag to `diff archive` command

[Lukas Wagner]

When --compare-content is set, the command will compare the
file content instead on relying on mtime to detect modified files.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
Changes from v2:
  - Increase buffersize to 4K
  - Added workaround for weird issue with tokio::try_join! and #[api]


 src/bin/proxmox_backup_debug/diff.rs   | 158 +++++++++++++++++++++++--
 src/bin/proxmox_backup_manager/ldap.rs | 102 ++++++++++++++++
 2 files changed, 248 insertions(+), 12 deletions(-)
 create mode 100644 src/bin/proxmox_backup_manager/ldap.rs

diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
index d5a4a1fe..a8a4e08d 100644
--- a/src/bin/proxmox_backup_debug/diff.rs
+++ b/src/bin/proxmox_backup_debug/diff.rs
@@ -28,12 +28,15 @@ use pbs_tools::json::required_string_param;
 use pxar::accessor::ReadAt;
 use pxar::EntryKind;
 use serde_json::Value;
+use tokio::io::AsyncReadExt;
 
 type ChunkDigest = [u8; 32];
 type FileEntry = pxar::accessor::aio::FileEntry<Arc<dyn ReadAt + Send + Sync>>;
 type Accessor = pxar::accessor::aio::Accessor<Arc<dyn ReadAt + Send + Sync>>;
 type Directory = pxar::accessor::aio::Directory<Arc<dyn ReadAt + Send + Sync>>;
 
+const BUFFERSIZE: usize = 4096;
+
 pub fn diff_commands() -> CommandLineInterface {
     let cmd_def = CliCommandMap::new().insert(
         "archive",
@@ -79,18 +82,30 @@ pub fn diff_commands() -> CommandLineInterface {
                 schema: KEYFD_SCHEMA,
                 optional: true,
             },
+            "compare-content": {
+                optional: true,
+                type: bool,
+                description: "Compare file content rather than solely relying on mtime for detecting modified files.",
+            },
         }
     }
 )]
 /// Diff an archive in two snapshots. The command will output a list of added, modified and deleted files.
-/// For modified files, only the file metadata (e.g. mtime, size, etc.) will be considered. The actual
-/// file contents will not be compared.
+/// For modified files, the file metadata (e.g. mode, uid, gid, size, etc.) will be considered. For detecting
+/// modification of file content, only mtime will be used by default. If the --compare-content flag is provided,
+/// mtime is ignored and file content will be compared.
 async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
     let repo = extract_repository_from_value(&param)?;
     let snapshot_a = required_string_param(&param, "prev-snapshot")?;
     let snapshot_b = required_string_param(&param, "snapshot")?;
     let archive_name = required_string_param(&param, "archive-name")?;
 
+    let compare_contents = match param.get("compare-content") {
+        Some(Value::Bool(value)) => *value,
+        Some(_) => bail!("invalid flag for compare-content"),
+        None => false,
+    };
+
     let namespace = match param.get("ns") {
         Some(Value::String(ns)) => ns.parse()?,
         Some(_) => bail!("invalid namespace parameter"),
@@ -120,7 +135,14 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
 
     if archive_name.ends_with(".pxar") {
         let file_name = format!("{}.didx", archive_name);
-        diff_archive(snapshot_a, snapshot_b, &file_name, &repo_params).await?;
+        diff_archive(
+            snapshot_a,
+            snapshot_b,
+            &file_name,
+            &repo_params,
+            compare_contents,
+        )
+        .await?;
     } else {
         bail!("Only .pxar files are supported");
     }
@@ -133,6 +155,7 @@ async fn diff_archive(
     snapshot_b: &str,
     file_name: &str,
     repo_params: &RepoParams,
+    compare_contents: bool,
 ) -> Result<(), Error> {
     let (index_a, accessor_a) = open_dynamic_index(snapshot_a, file_name, repo_params).await?;
     let (index_b, accessor_b) = open_dynamic_index(snapshot_b, file_name, repo_params).await?;
@@ -184,7 +207,7 @@ async fn diff_archive(
 
     // ... so we compare the file metadata/contents to narrow the selection down to files
     // which where *really* modified.
-    let modified_files = compare_files(potentially_modified).await?;
+    let modified_files = compare_files(potentially_modified, compare_contents).await?;
 
     show_file_list(&added_files, &deleted_files, &modified_files);
 
@@ -352,11 +375,12 @@ fn visit_directory<'f, 'c>(
 /// Check if files were actually modified
 async fn compare_files<'a>(
     files: HashMap<&'a OsStr, (&'a FileEntry, &'a FileEntry)>,
+    compare_contents: bool,
 ) -> Result<HashMap<&'a OsStr, (&'a FileEntry, ChangedProperties)>, Error> {
     let mut modified_files = HashMap::new();
 
     for (path, (entry_a, entry_b)) in files {
-        if let Some(changed) = compare_file(entry_a, entry_b).await? {
+        if let Some(changed) = compare_file(entry_a, entry_b, compare_contents).await? {
             modified_files.insert(path, (entry_b, changed));
         }
     }
@@ -367,6 +391,7 @@ async fn compare_files<'a>(
 async fn compare_file(
     file_a: &FileEntry,
     file_b: &FileEntry,
+    compare_contents: bool,
 ) -> Result<Option<ChangedProperties>, Error> {
     let mut changed = ChangedProperties::default();
 
@@ -385,10 +410,22 @@ async fn compare_file(
             changed.content = a.major != b.major || a.minor != b.minor
         }
         (EntryKind::File { size: size_a, .. }, EntryKind::File { size: size_b, .. }) => {
-            if size_a != size_b {
-                changed.size = true;
-                changed.content = true;
-            };
+            changed.size = size_a != size_b;
+
+            if compare_contents {
+                if changed.size {
+                    changed.content = true;
+                } else {
+                    let content_identical = compare_file_contents(file_a, file_b).await?;
+                    if content_identical && !changed.any_without_mtime() {
+                        // If the content is identical and nothing, exluding mtime,
+                        // has changed, we don't consider the entry as modified.
+                        changed.mtime = false;
+                    }
+
+                    changed.content = !content_identical;
+                }
+            }
         }
         (EntryKind::Directory, EntryKind::Directory) => {}
         (EntryKind::Socket, EntryKind::Socket) => {}
@@ -405,6 +442,45 @@ async fn compare_file(
     }
 }
 
+async fn compare_file_contents(file_a: &FileEntry, file_b: &FileEntry) -> Result<bool, Error> {
+    let mut contents_a = file_a.contents().await?;
+    let mut contents_b = file_b.contents().await?;
+
+    compare_readers(&mut contents_a, &mut contents_b).await
+}
+
+async fn compare_readers<T>(reader_a: &mut T, reader_b: &mut T) -> Result<bool, Error>
+where
+    T: AsyncReadExt + Unpin,
+{
+    let mut buf_a = Box::new([0u8; BUFFERSIZE]);
+    let mut buf_b = Box::new([0u8; BUFFERSIZE]);
+
+    loop {
+        // Put the both read calls into their own async blocks, otherwise
+        // tokio::try_join! in combination with our #[api] macro leads to some
+        // weird `higher-order lifetime error`
+        let read_fut_a = async { reader_a.read(buf_a.as_mut_slice()).await };
+        let read_fut_b = async { reader_b.read(buf_b.as_mut_slice()).await };
+
+        let (bytes_read_a, bytes_read_b) = tokio::try_join!(read_fut_a, read_fut_b)?;
+
+        if bytes_read_a != bytes_read_b {
+            return Ok(false);
+        }
+
+        if bytes_read_a == 0 {
+            break;
+        }
+
+        if buf_a[..bytes_read_a] != buf_b[..bytes_read_b] {
+            return Ok(false);
+        }
+    }
+
+    Ok(true)
+}
+
 #[derive(Copy, Clone, Default)]
 struct ChangedProperties {
     entry_type: bool,
@@ -438,8 +514,11 @@ impl ChangedProperties {
     }
 
     fn any(&self) -> bool {
+        self.any_without_mtime() || self.mtime
+    }
+
+    fn any_without_mtime(&self) -> bool {
         self.entry_type
-            || self.mtime
             || self.acl
             || self.xattrs
             || self.fcaps
@@ -475,9 +554,10 @@ fn format_filesize(entry: &FileEntry, changed: bool) -> String {
 fn format_mtime(entry: &FileEntry, changed: bool) -> String {
     let mtime = &entry.metadata().stat.mtime;
 
-    let format = if changed { "*%F %T" } else { " %F %T" };
+    let mut format = change_indicator(changed).to_owned();
+    format.push_str("%F %T");
 
-    proxmox_time::strftime_local(format, mtime.secs).unwrap_or_default()
+    proxmox_time::strftime_local(&format, mtime.secs).unwrap_or_default()
 }
 
 fn format_mode(entry: &FileEntry, changed: bool) -> String {
@@ -553,3 +633,57 @@ fn show_file_list(
         println!("{op} {entry_type:>2} {mode:>5} {uid:>6} {gid:>6} {size:>10} {mtime:11} {name}");
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    use std::{
+        io::Cursor,
+        pin::Pin,
+        task::{Context, Poll},
+    };
+    use tokio::io::{AsyncRead, ReadBuf};
+
+    struct MockedAsyncReader(Cursor<Vec<u8>>);
+
+    impl AsyncRead for MockedAsyncReader {
+        fn poll_read(
+            self: Pin<&mut Self>,
+            _cx: &mut Context<'_>,
+            read_buf: &mut ReadBuf<'_>,
+        ) -> Poll<std::io::Result<()>> {
+            let mut buf = vec![0u8; 100];
+
+            let res = std::io::Read::read(&mut self.get_mut().0, &mut buf);
+
+            if let Ok(bytes) = res {
+                read_buf.put_slice(&buf[..bytes]);
+            }
+
+            Poll::Ready(res.map(|_| ()))
+        }
+    }
+
+    #[test]
+    fn test_do_compare_file_contents() {
+        fn compare(a: Vec<u8>, b: Vec<u8>) -> Result<bool, Error> {
+            let mut mock_a = MockedAsyncReader(Cursor::new(a));
+            let mut mock_b = MockedAsyncReader(Cursor::new(b));
+
+            proxmox_async::runtime::block_on(compare_readers(&mut mock_a, &mut mock_b))
+        }
+
+        assert!(matches!(compare(vec![0; 15], vec![0; 15]), Ok(true)));
+        assert!(matches!(compare(vec![0; 15], vec![0; 14]), Ok(false)));
+        assert!(matches!(compare(vec![0; 15], vec![1; 15]), Ok(false)));
+
+        let mut buf = vec![1u8; 2 * BUFFERSIZE];
+        buf[BUFFERSIZE] = 0;
+        assert!(matches!(compare(vec![1u8; 2 * BUFFERSIZE], buf), Ok(false)));
+
+        let mut buf = vec![1u8; 2 * BUFFERSIZE];
+        buf[2 * BUFFERSIZE - 1] = 0;
+        assert!(matches!(compare(vec![1u8; 2 * BUFFERSIZE], buf), Ok(false)));
+    }
+}
diff --git a/src/bin/proxmox_backup_manager/ldap.rs b/src/bin/proxmox_backup_manager/ldap.rs
new file mode 100644
index 00000000..a2f10f66
--- /dev/null
+++ b/src/bin/proxmox_backup_manager/ldap.rs
@@ -0,0 +1,102 @@
+use anyhow::Error;
+use serde_json::Value;
+
+use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
+use proxmox_schema::api;
+
+use pbs_api_types::REALM_ID_SCHEMA;
+
+use proxmox_backup::api2;
+
+#[api(
+    input: {
+        properties: {
+            "output-format": {
+                schema: OUTPUT_FORMAT,
+                optional: true,
+            },
+        }
+    }
+)]
+/// List configured LDAP realms
+fn list_ldap_realms(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+    let output_format = get_output_format(&param);
+
+    let info = &api2::config::access::ldap::API_METHOD_LIST_LDAP_REALMS;
+    let mut data = match info.handler {
+        ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
+        _ => unreachable!(),
+    };
+
+    let options = default_table_format_options()
+        .column(ColumnConfig::new("realm"))
+        // .column(ColumnConfig::new("issuer-url"))
+        .column(ColumnConfig::new("comment"));
+
+    format_and_print_result_full(&mut data, &info.returns, &output_format, &options);
+
+    Ok(Value::Null)
+}
+#[api(
+    input: {
+        properties: {
+            realm: {
+                schema: REALM_ID_SCHEMA,
+            },
+            "output-format": {
+                schema: OUTPUT_FORMAT,
+                optional: true,
+            },
+        }
+    }
+)]
+
+/// Show LDAP realm configuration
+fn show_ldap_realm(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+    let output_format = get_output_format(&param);
+
+    let info = &api2::config::access::ldap::API_METHOD_READ_LDAP_REALM;
+    let mut data = match info.handler {
+        ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
+        _ => unreachable!(),
+    };
+
+    let options = default_table_format_options();
+    format_and_print_result_full(&mut data, &info.returns, &output_format, &options);
+
+    Ok(Value::Null)
+}
+
+pub fn ldap_commands() -> CommandLineInterface {
+    let cmd_def = CliCommandMap::new()
+        .insert("list", CliCommand::new(&API_METHOD_LIST_LDAP_REALMS))
+        .insert(
+            "show",
+            CliCommand::new(&API_METHOD_SHOW_LDAP_REALM)
+                .arg_param(&["realm"])
+                .completion_cb("realm", pbs_config::domains::complete_ldap_realm_name),
+        )
+        .insert(
+            "create",
+            CliCommand::new(&api2::config::access::ldap::API_METHOD_CREATE_LDAP_REALM)
+                .arg_param(&["realm"])
+                .arg_param(&["realm"])
+                .completion_cb("realm", pbs_config::domains::complete_ldap_realm_name),
+        )
+        .insert(
+            "update",
+            CliCommand::new(&api2::config::access::ldap::API_METHOD_UPDATE_LDAP_REALM)
+                .arg_param(&["realm"])
+                .arg_param(&["realm"])
+                .completion_cb("realm", pbs_config::domains::complete_ldap_realm_name),
+        )
+        .insert(
+            "delete",
+            CliCommand::new(&api2::config::access::ldap::API_METHOD_DELETE_LDAP_REALM)
+                .arg_param(&["realm"])
+                .arg_param(&["realm"])
+                .completion_cb("realm", pbs_config::domains::complete_ldap_realm_name),
+        );
+
+    cmd_def.into()
+}
-- 
2.30.2

[pbs-devel] [PATCH v3 proxmox-backup 3/4] debug cli: add colored output for `diff archive`

[Lukas Wagner]

This commit adds the `--color` flag to the `diff archive` tool.
Valid values are `always`, `auto` and `never`. `always` and
`never` should be self-explanatory, whereas `auto` will enable
colors unless one of the following is true:
  - STDOUT is not a tty
  - TERM=dumb is set
  - NO_COLOR is set

The tool will highlight changed file attributes in yellow.
Furthermore, (A)dded files are highlighted in green,
(M)odified in yellow and (D)eleted in red.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---

Changes from v2:
  - Drop dependency on `atty` crate
  - Remove modificiations to debian/control

 Cargo.toml                           |   1 +
 src/bin/proxmox_backup_debug/diff.rs | 371 ++++++++++++++++++++++-----
 2 files changed, 306 insertions(+), 66 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 8ee48127..7fac1bfa 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -75,6 +75,7 @@ serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"
 siphasher = "0.3"
 syslog = "4.0"
+termcolor = "1.1.2"
 tokio = { version = "1.6", features = [ "fs", "io-util", "io-std", "macros", "net", "parking_lot", "process", "rt", "rt-multi-thread", "signal", "time" ] }
 tokio-openssl = "0.6.1"
 tokio-stream = "0.1.0"
diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
index a8a4e08d..2149720f 100644
--- a/src/bin/proxmox_backup_debug/diff.rs
+++ b/src/bin/proxmox_backup_debug/diff.rs
@@ -1,5 +1,6 @@
 use std::collections::{HashMap, HashSet};
 use std::ffi::{OsStr, OsString};
+use std::io::Write;
 use std::iter::FromIterator;
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
@@ -28,6 +29,8 @@ use pbs_tools::json::required_string_param;
 use pxar::accessor::ReadAt;
 use pxar::EntryKind;
 use serde_json::Value;
+
+use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};
 use tokio::io::AsyncReadExt;
 
 type ChunkDigest = [u8; 32];
@@ -87,6 +90,11 @@ pub fn diff_commands() -> CommandLineInterface {
                 type: bool,
                 description: "Compare file content rather than solely relying on mtime for detecting modified files.",
             },
+            "color": {
+                optional: true,
+                type: String,
+                description: "Set mode for colored output. Can be `always`, `auto` or `never`. `auto` will display colors only if stdout is a tty. Defaults to `auto`."
+            }
         }
     }
 )]
@@ -106,6 +114,12 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
         None => false,
     };
 
+    let color = match param.get("color") {
+        Some(Value::String(color)) => color.as_str().try_into()?,
+        Some(_) => bail!("invalid color parameter. Valid choices are `always`, `auto` and `never`"),
+        None => ColorMode::Auto,
+    };
+
     let namespace = match param.get("ns") {
         Some(Value::String(ns)) => ns.parse()?,
         Some(_) => bail!("invalid namespace parameter"),
@@ -133,6 +147,8 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
         namespace,
     };
 
+    let output_params = OutputParams { color };
+
     if archive_name.ends_with(".pxar") {
         let file_name = format!("{}.didx", archive_name);
         diff_archive(
@@ -141,6 +157,7 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
             &file_name,
             &repo_params,
             compare_contents,
+            &output_params,
         )
         .await?;
     } else {
@@ -156,6 +173,7 @@ async fn diff_archive(
     file_name: &str,
     repo_params: &RepoParams,
     compare_contents: bool,
+    output_params: &OutputParams,
 ) -> Result<(), Error> {
     let (index_a, accessor_a) = open_dynamic_index(snapshot_a, file_name, repo_params).await?;
     let (index_b, accessor_b) = open_dynamic_index(snapshot_b, file_name, repo_params).await?;
@@ -209,17 +227,40 @@ async fn diff_archive(
     // which where *really* modified.
     let modified_files = compare_files(potentially_modified, compare_contents).await?;
 
-    show_file_list(&added_files, &deleted_files, &modified_files);
+    show_file_list(&added_files, &deleted_files, &modified_files, output_params)?;
 
     Ok(())
 }
 
+enum ColorMode {
+    Always,
+    Auto,
+    Never,
+}
+
+impl TryFrom<&str> for ColorMode {
+    type Error = Error;
+
+    fn try_from(value: &str) -> Result<Self, Self::Error> {
+        match value {
+            "auto" => Ok(Self::Auto),
+            "always" => Ok(Self::Always),
+            "never" => Ok(Self::Never),
+            _ => bail!("invalid color parameter. Valid choices are `always`, `auto` and `never`"),
+        }
+    }
+}
+
 struct RepoParams {
     repo: BackupRepository,
     crypt_config: Option<Arc<CryptConfig>>,
     namespace: BackupNamespace,
 }
 
+struct OutputParams {
+    color: ColorMode,
+}
+
 async fn open_dynamic_index(
     snapshot: &str,
     archive_name: &str,
@@ -531,70 +572,271 @@ impl ChangedProperties {
     }
 }
 
-fn change_indicator(changed: bool) -> &'static str {
-    if changed {
-        "*"
-    } else {
-        " "
-    }
+enum FileOperation {
+    Added,
+    Modified,
+    Deleted,
 }
 
-fn format_filesize(entry: &FileEntry, changed: bool) -> String {
-    if let Some(size) = entry.file_size() {
-        format!(
-            "{}{:.1}",
-            change_indicator(changed),
-            HumanByte::new_decimal(size as f64)
-        )
-    } else {
-        String::new()
+struct ColumnWidths {
+    operation: usize,
+    entry_type: usize,
+    uid: usize,
+    gid: usize,
+    mode: usize,
+    filesize: usize,
+    mtime: usize,
+}
+
+impl Default for ColumnWidths {
+    fn default() -> Self {
+        Self {
+            operation: 1,
+            entry_type: 2,
+            uid: 6,
+            gid: 6,
+            mode: 6,
+            filesize: 10,
+            mtime: 11,
+        }
     }
 }
 
-fn format_mtime(entry: &FileEntry, changed: bool) -> String {
-    let mtime = &entry.metadata().stat.mtime;
+struct FileEntryPrinter {
+    stream: StandardStream,
+    column_widths: ColumnWidths,
+    changed_color: Color,
+}
 
-    let mut format = change_indicator(changed).to_owned();
-    format.push_str("%F %T");
+impl FileEntryPrinter {
+    pub fn new(output_params: &OutputParams) -> Self {
+        let color_choice = match output_params.color {
+            ColorMode::Always => ColorChoice::Always,
+            ColorMode::Auto => {
+                if unsafe { libc::isatty(1) == 1 } {
+                    // Show colors unless `TERM=dumb` or `NO_COLOR` is set.
+                    ColorChoice::Auto
+                } else {
+                    ColorChoice::Never
+                }
+            }
+            ColorMode::Never => ColorChoice::Never,
+        };
 
-    proxmox_time::strftime_local(&format, mtime.secs).unwrap_or_default()
-}
+        let stdout = StandardStream::stdout(color_choice);
 
-fn format_mode(entry: &FileEntry, changed: bool) -> String {
-    let mode = entry.metadata().stat.mode & 0o7777;
-    format!("{}{:o}", change_indicator(changed), mode)
-}
+        Self {
+            stream: stdout,
+            column_widths: ColumnWidths::default(),
+            changed_color: Color::Yellow,
+        }
+    }
 
-fn format_entry_type(entry: &FileEntry, changed: bool) -> String {
-    let kind = match entry.kind() {
-        EntryKind::Symlink(_) => "l",
-        EntryKind::Hardlink(_) => "h",
-        EntryKind::Device(_) if entry.metadata().stat.is_blockdev() => "b",
-        EntryKind::Device(_) => "c",
-        EntryKind::Socket => "s",
-        EntryKind::Fifo => "p",
-        EntryKind::File { .. } => "f",
-        EntryKind::Directory => "d",
-        _ => " ",
-    };
+    fn change_indicator(&self, changed: bool) -> &'static str {
+        if changed {
+            "*"
+        } else {
+            " "
+        }
+    }
 
-    format!("{}{}", change_indicator(changed), kind)
-}
+    fn set_color_if_changed(&mut self, changed: bool) -> Result<(), Error> {
+        if changed {
+            self.stream
+                .set_color(ColorSpec::new().set_fg(Some(self.changed_color)))?;
+        }
 
-fn format_uid(entry: &FileEntry, changed: bool) -> String {
-    format!("{}{}", change_indicator(changed), entry.metadata().stat.uid)
-}
+        Ok(())
+    }
 
-fn format_gid(entry: &FileEntry, changed: bool) -> String {
-    format!("{}{}", change_indicator(changed), entry.metadata().stat.gid)
-}
+    fn write_operation(&mut self, op: FileOperation) -> Result<(), Error> {
+        let (text, color) = match op {
+            FileOperation::Added => ("A", Color::Green),
+            FileOperation::Modified => ("M", Color::Yellow),
+            FileOperation::Deleted => ("D", Color::Red),
+        };
 
-fn format_file_name(entry: &FileEntry, changed: bool) -> String {
-    format!(
-        "{}{}",
-        change_indicator(changed),
-        entry.file_name().to_string_lossy()
-    )
+        self.stream
+            .set_color(ColorSpec::new().set_fg(Some(color)))?;
+
+        write!(
+            self.stream,
+            "{text:>width$}",
+            width = self.column_widths.operation,
+        )?;
+
+        self.stream.reset()?;
+
+        Ok(())
+    }
+
+    fn write_filesize(&mut self, entry: &FileEntry, changed: bool) -> Result<(), Error> {
+        let output = if let Some(size) = entry.file_size() {
+            format!(
+                "{}{:.1}",
+                self.change_indicator(changed),
+                HumanByte::new_decimal(size as f64)
+            )
+        } else {
+            String::new()
+        };
+
+        self.set_color_if_changed(changed)?;
+        write!(
+            self.stream,
+            "{output:>width$}",
+            width = self.column_widths.filesize,
+        )?;
+        self.stream.reset()?;
+
+        Ok(())
+    }
+
+    fn write_mtime(&mut self, entry: &FileEntry, changed: bool) -> Result<(), Error> {
+        let mtime = &entry.metadata().stat.mtime;
+
+        let mut format = self.change_indicator(changed).to_owned();
+        format.push_str("%F %T");
+
+        let output = proxmox_time::strftime_local(&format, mtime.secs).unwrap_or_default();
+
+        self.set_color_if_changed(changed)?;
+        write!(
+            self.stream,
+            "{output:>width$}",
+            width = self.column_widths.mtime,
+        )?;
+        self.stream.reset()?;
+
+        Ok(())
+    }
+
+    fn write_mode(&mut self, entry: &FileEntry, changed: bool) -> Result<(), Error> {
+        let mode = entry.metadata().stat.mode & 0o7777;
+        let output = format!("{}{:o}", self.change_indicator(changed), mode);
+
+        self.set_color_if_changed(changed)?;
+        write!(
+            self.stream,
+            "{output:>width$}",
+            width = self.column_widths.mode,
+        )?;
+        self.stream.reset()?;
+
+        Ok(())
+    }
+
+    fn write_entry_type(&mut self, entry: &FileEntry, changed: bool) -> Result<(), Error> {
+        let kind = match entry.kind() {
+            EntryKind::Symlink(_) => "l",
+            EntryKind::Hardlink(_) => "h",
+            EntryKind::Device(_) if entry.metadata().stat.is_blockdev() => "b",
+            EntryKind::Device(_) => "c",
+            EntryKind::Socket => "s",
+            EntryKind::Fifo => "p",
+            EntryKind::File { .. } => "f",
+            EntryKind::Directory => "d",
+            _ => " ",
+        };
+
+        let output = format!("{}{}", self.change_indicator(changed), kind);
+
+        self.set_color_if_changed(changed)?;
+        write!(
+            self.stream,
+            "{output:>width$}",
+            width = self.column_widths.entry_type,
+        )?;
+        self.stream.reset()?;
+
+        Ok(())
+    }
+
+    fn write_uid(&mut self, entry: &FileEntry, changed: bool) -> Result<(), Error> {
+        let output = format!(
+            "{}{}",
+            self.change_indicator(changed),
+            entry.metadata().stat.uid
+        );
+
+        self.set_color_if_changed(changed)?;
+        write!(
+            self.stream,
+            "{output:>width$}",
+            width = self.column_widths.uid,
+        )?;
+        self.stream.reset()?;
+        Ok(())
+    }
+
+    fn write_gid(&mut self, entry: &FileEntry, changed: bool) -> Result<(), Error> {
+        let output = format!(
+            "{}{}",
+            self.change_indicator(changed),
+            entry.metadata().stat.gid
+        );
+
+        self.set_color_if_changed(changed)?;
+        write!(
+            self.stream,
+            "{output:>width$}",
+            width = self.column_widths.gid,
+        )?;
+        self.stream.reset()?;
+        Ok(())
+    }
+
+    fn write_file_name(&mut self, entry: &FileEntry, changed: bool) -> Result<(), Error> {
+        self.set_color_if_changed(changed)?;
+        write!(
+            self.stream,
+            "{}{}",
+            self.change_indicator(changed),
+            entry.file_name().to_string_lossy()
+        )?;
+        self.stream.reset()?;
+
+        Ok(())
+    }
+
+    fn write_column_seperator(&mut self) -> Result<(), Error> {
+        write!(self.stream, " ")?;
+        Ok(())
+    }
+
+    /// Print a file entry, including `changed` indicators and column seperators
+    pub fn print_file_entry(
+        &mut self,
+        entry: &FileEntry,
+        changed: &ChangedProperties,
+        operation: FileOperation,
+    ) -> Result<(), Error> {
+        self.write_operation(operation)?;
+        self.write_column_seperator()?;
+
+        self.write_entry_type(entry, changed.entry_type)?;
+        self.write_column_seperator()?;
+
+        self.write_uid(entry, changed.uid)?;
+        self.write_column_seperator()?;
+
+        self.write_gid(entry, changed.gid)?;
+        self.write_column_seperator()?;
+
+        self.write_mode(entry, changed.mode)?;
+        self.write_column_seperator()?;
+
+        self.write_filesize(entry, changed.size)?;
+        self.write_column_seperator()?;
+
+        self.write_mtime(entry, changed.mtime)?;
+        self.write_column_seperator()?;
+
+        self.write_file_name(entry, changed.content)?;
+        writeln!(self.stream)?;
+
+        Ok(())
+    }
 }
 
 /// Display a sorted list of added, modified, deleted files.
@@ -602,7 +844,8 @@ fn show_file_list(
     added: &HashMap<&OsStr, &FileEntry>,
     deleted: &HashMap<&OsStr, &FileEntry>,
     modified: &HashMap<&OsStr, (&FileEntry, ChangedProperties)>,
-) {
+    output_params: &OutputParams,
+) -> Result<(), Error> {
     let mut all: Vec<&OsStr> = Vec::new();
 
     all.extend(added.keys());
@@ -611,27 +854,23 @@ fn show_file_list(
 
     all.sort();
 
+    let mut printer = FileEntryPrinter::new(output_params);
+
     for file in all {
-        let (op, entry, changed) = if let Some(entry) = added.get(file) {
-            ("A", entry, ChangedProperties::default())
+        let (operation, entry, changed) = if let Some(entry) = added.get(file) {
+            (FileOperation::Added, entry, ChangedProperties::default())
         } else if let Some(entry) = deleted.get(file) {
-            ("D", entry, ChangedProperties::default())
+            (FileOperation::Deleted, entry, ChangedProperties::default())
         } else if let Some((entry, changed)) = modified.get(file) {
-            ("M", entry, *changed)
+            (FileOperation::Modified, entry, *changed)
         } else {
             unreachable!();
         };
 
-        let entry_type = format_entry_type(entry, changed.entry_type);
-        let uid = format_uid(entry, changed.uid);
-        let gid = format_gid(entry, changed.gid);
-        let mode = format_mode(entry, changed.mode);
-        let size = format_filesize(entry, changed.size);
-        let mtime = format_mtime(entry, changed.mtime);
-        let name = format_file_name(entry, changed.content);
-
-        println!("{op} {entry_type:>2} {mode:>5} {uid:>6} {gid:>6} {size:>10} {mtime:11} {name}");
+        printer.print_file_entry(entry, &changed, operation)?;
     }
+
+    Ok(())
 }
 
 #[cfg(test)]
-- 
2.30.2

[pbs-devel] [PATCH v3 proxmox-backup 4/4] debug cli: move parameters into the function signature

[Lukas Wagner]

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
Changes from v2:
  - This commit is new in v3: `diff_archive_cmd`: Moved parameters into the
    function signature, instead of manually extracting it from `Value`.

 src/bin/proxmox_backup_debug/diff.rs | 65 +++++++++++-----------------
 1 file changed, 25 insertions(+), 40 deletions(-)

diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
index 2149720f..3160efb1 100644
--- a/src/bin/proxmox_backup_debug/diff.rs
+++ b/src/bin/proxmox_backup_debug/diff.rs
@@ -25,9 +25,9 @@ use pbs_config::key_config::decrypt_key;
 use pbs_datastore::dynamic_index::{BufferedDynamicReader, DynamicIndexReader, LocalDynamicReadAt};
 use pbs_datastore::index::IndexFile;
 use pbs_tools::crypt_config::CryptConfig;
-use pbs_tools::json::required_string_param;
 use pxar::accessor::ReadAt;
 use pxar::EntryKind;
+use serde::Deserialize;
 use serde_json::Value;
 
 use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};
@@ -92,8 +92,7 @@ pub fn diff_commands() -> CommandLineInterface {
             },
             "color": {
                 optional: true,
-                type: String,
-                description: "Set mode for colored output. Can be `always`, `auto` or `never`. `auto` will display colors only if stdout is a tty. Defaults to `auto`."
+                type: ColorMode,
             }
         }
     }
@@ -102,29 +101,20 @@ pub fn diff_commands() -> CommandLineInterface {
 /// For modified files, the file metadata (e.g. mode, uid, gid, size, etc.) will be considered. For detecting
 /// modification of file content, only mtime will be used by default. If the --compare-content flag is provided,
 /// mtime is ignored and file content will be compared.
-async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
+async fn diff_archive_cmd(
+    prev_snapshot: String,
+    snapshot: String,
+    archive_name: String,
+    compare_content: Option<bool>,
+    color: Option<ColorMode>,
+    ns: Option<BackupNamespace>,
+    param: Value,
+) -> Result<(), Error> {
     let repo = extract_repository_from_value(&param)?;
-    let snapshot_a = required_string_param(&param, "prev-snapshot")?;
-    let snapshot_b = required_string_param(&param, "snapshot")?;
-    let archive_name = required_string_param(&param, "archive-name")?;
-
-    let compare_contents = match param.get("compare-content") {
-        Some(Value::Bool(value)) => *value,
-        Some(_) => bail!("invalid flag for compare-content"),
-        None => false,
-    };
 
-    let color = match param.get("color") {
-        Some(Value::String(color)) => color.as_str().try_into()?,
-        Some(_) => bail!("invalid color parameter. Valid choices are `always`, `auto` and `never`"),
-        None => ColorMode::Auto,
-    };
-
-    let namespace = match param.get("ns") {
-        Some(Value::String(ns)) => ns.parse()?,
-        Some(_) => bail!("invalid namespace parameter"),
-        None => BackupNamespace::root(),
-    };
+    let compare_content = compare_content.unwrap_or(false);
+    let color = color.unwrap_or_default();
+    let namespace = ns.unwrap_or_else(BackupNamespace::root);
 
     let crypto = crypto_parameters(&param)?;
 
@@ -152,11 +142,11 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
     if archive_name.ends_with(".pxar") {
         let file_name = format!("{}.didx", archive_name);
         diff_archive(
-            snapshot_a,
-            snapshot_b,
+            &prev_snapshot,
+            &snapshot,
             &file_name,
             &repo_params,
-            compare_contents,
+            compare_content,
             &output_params,
         )
         .await?;
@@ -232,25 +222,20 @@ async fn diff_archive(
     Ok(())
 }
 
+#[api(default: "auto")]
+#[derive(Default, Deserialize)]
+#[serde(rename_all = "lowercase")]
+/// Color output options
 enum ColorMode {
+    /// Always output colors
     Always,
+    /// Output colors if STDOUT is a tty and neither of TERM=dumb or NO_COLOR is set
+    #[default]
     Auto,
+    /// Never output colors
     Never,
 }
 
-impl TryFrom<&str> for ColorMode {
-    type Error = Error;
-
-    fn try_from(value: &str) -> Result<Self, Self::Error> {
-        match value {
-            "auto" => Ok(Self::Auto),
-            "always" => Ok(Self::Always),
-            "never" => Ok(Self::Never),
-            _ => bail!("invalid color parameter. Valid choices are `always`, `auto` and `never`"),
-        }
-    }
-}
-
 struct RepoParams {
     repo: BackupRepository,
     crypt_config: Option<Arc<CryptConfig>>,
-- 
2.30.2

Re: [pbs-devel] [PATCH v3 proxmox-backup 1/4] debug cli: show more file attributes for `diff archive` command

[Wolfgang Bumiller]

On Wed, Dec 07, 2022 at 10:38:16AM +0100, Lukas Wagner wrote:
> This commit enriches the output of the `diff archive` command,
> showing pxar entry type, mode, uid, gid, size, mtime and filename.
> Attributes that changed between both snapshots are prefixed
> with a "*".
> 
> For instance:
> 
> $ proxmox-backup-debug diff archive ...
> A  f   644  10045  10000    0 B  2022-11-28 13:44:51  add.txt
> M  f   644  10045  10000    6 B *2022-11-28 13:45:05  content.txt
> D  f   644  10045  10000    0 B  2022-11-28 13:17:09  deleted.txt
> M  f   644  10045    *29    0 B  2022-11-28 13:16:20  gid.txt
> M  f  *777  10045  10000    0 B  2022-11-28 13:42:47  mode.txt
> M  f   644  10045  10000    0 B *2022-11-28 13:44:33  mtime.txt
> M  f   644  10045  10000   *7 B *2022-11-28 13:44:59 *size.txt
> M  f   644 *64045  10000    0 B  2022-11-28 13:16:18  uid.txt
> M *f   644  10045  10000   10 B  2022-11-28 13:44:59  type_changed.txt
> 
> Also, this commit ensures that we always show the *new* type.
> Previously, the command showed the old type if it was changed.
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  src/bin/proxmox_backup_debug/diff.rs | 221 ++++++++++++++++++++-------
>  1 file changed, 167 insertions(+), 54 deletions(-)
> 
> diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
> index cb157ec2..d5a4a1fe 100644
> --- a/src/bin/proxmox_backup_debug/diff.rs
> +++ b/src/bin/proxmox_backup_debug/diff.rs
> @@ -11,7 +11,7 @@ use futures::FutureExt;
>  use proxmox_router::cli::{CliCommand, CliCommandMap, CommandLineInterface};
>  use proxmox_schema::api;
>  
> -use pbs_api_types::{BackupNamespace, BackupPart};
> +use pbs_api_types::{BackupNamespace, BackupPart, HumanByte};
>  use pbs_client::tools::key_source::{
>      crypto_parameters, format_key_source, get_encryption_key_password, KEYFD_SCHEMA,
>  };
> @@ -173,15 +173,18 @@ async fn diff_archive(
>      // If file is present in both snapshots, it *might* be modified, but does not have to be.
>      // If another, unmodified file resides in the same chunk as an actually modified one,
>      // it will also show up as modified here...
> -    let potentially_modified: HashMap<&OsStr, &FileEntry> = files_in_a
> +    let potentially_modified: HashMap<&OsStr, (&FileEntry, &FileEntry)> = files_in_a
>          .iter()
> -        .filter(|(path, _)| files_in_b.contains_key(*path))
> -        .map(|(path, entry)| (path.as_os_str(), entry))
> +        .filter_map(|(path, entry_a)| {
> +            files_in_b
> +                .get(path)
> +                .map(|entry_b| (path.as_os_str(), (entry_a, entry_b)))
> +        })
>          .collect();
>  
>      // ... so we compare the file metadata/contents to narrow the selection down to files
>      // which where *really* modified.
> -    let modified_files = compare_files(&files_in_a, &files_in_b, potentially_modified).await?;
> +    let modified_files = compare_files(potentially_modified).await?;
>  
>      show_file_list(&added_files, &deleted_files, &modified_files);
>  
> @@ -335,7 +338,6 @@ fn visit_directory<'f, 'c>(
>                  let digest = chunk_list.get(chunk_index).context("Invalid chunk index")?;
>  
>                  if chunk_diff.get(digest).is_some() {
> -                    // files.insert(file_path.clone().into_os_string());
>                      entries.insert(file_path.into_os_string(), entry);
>                      break;
>                  }
> @@ -349,62 +351,177 @@ fn visit_directory<'f, 'c>(
>  
>  /// Check if files were actually modified
>  async fn compare_files<'a>(
> -    entries_a: &HashMap<OsString, FileEntry>,
> -    entries_b: &HashMap<OsString, FileEntry>,
> -    files: HashMap<&'a OsStr, &'a FileEntry>,
> -) -> Result<HashMap<&'a OsStr, &'a FileEntry>, Error> {
> +    files: HashMap<&'a OsStr, (&'a FileEntry, &'a FileEntry)>,
> +) -> Result<HashMap<&'a OsStr, (&'a FileEntry, ChangedProperties)>, Error> {
>      let mut modified_files = HashMap::new();
>  
> -    for (path, entry) in files {
> -        let p = path.to_os_string();
> -        let file_a = entries_a.get(&p).context("File entry not in map")?;
> -        let file_b = entries_b.get(&p).context("File entry not in map")?;
> -
> -        if !compare_file(file_a, file_b).await {
> -            modified_files.insert(path, entry);
> +    for (path, (entry_a, entry_b)) in files {
> +        if let Some(changed) = compare_file(entry_a, entry_b).await? {
> +            modified_files.insert(path, (entry_b, changed));
>          }
>      }
>  
>      Ok(modified_files)
>  }
>  
> -async fn compare_file(file_a: &FileEntry, file_b: &FileEntry) -> bool {
> -    if file_a.metadata() != file_b.metadata() {
> -        // Check if mtime, permissions, ACLs, etc. have changed - if they have changed, we consider
> -        // the file as modified.
> -        return false;
> -    }
> +async fn compare_file(
> +    file_a: &FileEntry,
> +    file_b: &FileEntry,
> +) -> Result<Option<ChangedProperties>, Error> {
> +    let mut changed = ChangedProperties::default();
> +
> +    changed.set_from_metadata(file_a, file_b);
>  
>      match (file_a.kind(), file_b.kind()) {
>          (EntryKind::Symlink(a), EntryKind::Symlink(b)) => {
>              // Check whether the link target has changed.
> -            a.as_os_str() == b.as_os_str()
> +            changed.content = a.as_os_str() != b.as_os_str();
>          }
>          (EntryKind::Hardlink(a), EntryKind::Hardlink(b)) => {
>              // Check whether the link target has changed.
> -            a.as_os_str() == b.as_os_str()
> +            changed.content = a.as_os_str() != b.as_os_str();
> +        }
> +        (EntryKind::Device(a), EntryKind::Device(b)) => {
> +            changed.content = a.major != b.major || a.minor != b.minor

Just noticed - we might be missing a change between blockdev/chardev
here? Might need to include

    file_a.metadata().stat.is_blockdev() != file_b.metadata().stat.is_blockdev()

as well? Or am I missing this being handled elsewhere already?

>          }
> -        (EntryKind::Device(a), EntryKind::Device(b)) => a.major == b.major && a.minor == b.minor,
> -        (EntryKind::Socket, EntryKind::Socket) => true,
> -        (EntryKind::Fifo, EntryKind::Fifo) => true,
>          (EntryKind::File { size: size_a, .. }, EntryKind::File { size: size_b, .. }) => {
> -            // At this point we know that all metadata including mtime is
> -            // the same. To speed things up, we consider the files as equal if they also have
> -            // the same size.
> -            // If one were completely paranoid, one could compare the actual file contents,
> -            // but this decreases performance drastically.
> -            size_a == size_b
> +            if size_a != size_b {
> +                changed.size = true;
> +                changed.content = true;
> +            };
> +        }
> +        (EntryKind::Directory, EntryKind::Directory) => {}
> +        (EntryKind::Socket, EntryKind::Socket) => {}
> +        (EntryKind::Fifo, EntryKind::Fifo) => {}
> +        (_, _) => {
> +            changed.entry_type = true;
>          }
> -        (EntryKind::Directory, EntryKind::Directory) => true,
> -        (_, _) => false, // Kind has changed, so we of course consider it modified.
> +    }
> +
> +    if changed.any() {
> +        Ok(Some(changed))
> +    } else {
> +        Ok(None)
> +    }
> +}
> +
> +#[derive(Copy, Clone, Default)]
> +struct ChangedProperties {
> +    entry_type: bool,
> +    mtime: bool,
> +    acl: bool,
> +    xattrs: bool,
> +    fcaps: bool,
> +    quota_project_id: bool,
> +    mode: bool,
> +    flags: bool,
> +    uid: bool,
> +    gid: bool,
> +    size: bool,
> +    content: bool,
> +}
> +
> +impl ChangedProperties {
> +    fn set_from_metadata(&mut self, file_a: &FileEntry, file_b: &FileEntry) {
> +        let a = file_a.metadata();
> +        let b = file_b.metadata();
> +
> +        self.acl = a.acl != b.acl;
> +        self.xattrs = a.xattrs != b.xattrs;
> +        self.fcaps = a.fcaps != b.fcaps;
> +        self.quota_project_id = a.quota_project_id != b.quota_project_id;
> +        self.mode = a.stat.mode != b.stat.mode;
> +        self.flags = a.stat.flags != b.stat.flags;
> +        self.uid = a.stat.uid != b.stat.uid;
> +        self.gid = a.stat.gid != b.stat.gid;
> +        self.mtime = a.stat.mtime != b.stat.mtime;
> +    }
> +
> +    fn any(&self) -> bool {
> +        self.entry_type
> +            || self.mtime
> +            || self.acl
> +            || self.xattrs
> +            || self.fcaps
> +            || self.quota_project_id
> +            || self.mode
> +            || self.flags
> +            || self.uid
> +            || self.gid
> +            || self.content
> +    }
> +}
> +
> +fn change_indicator(changed: bool) -> &'static str {
> +    if changed {
> +        "*"
> +    } else {
> +        " "
> +    }
> +}
> +
> +fn format_filesize(entry: &FileEntry, changed: bool) -> String {
> +    if let Some(size) = entry.file_size() {
> +        format!(
> +            "{}{:.1}",
> +            change_indicator(changed),
> +            HumanByte::new_decimal(size as f64)
> +        )
> +    } else {
> +        String::new()
>      }
>  }
>  
> +fn format_mtime(entry: &FileEntry, changed: bool) -> String {
> +    let mtime = &entry.metadata().stat.mtime;
> +
> +    let format = if changed { "*%F %T" } else { " %F %T" };
> +
> +    proxmox_time::strftime_local(format, mtime.secs).unwrap_or_default()
> +}
> +
> +fn format_mode(entry: &FileEntry, changed: bool) -> String {
> +    let mode = entry.metadata().stat.mode & 0o7777;
> +    format!("{}{:o}", change_indicator(changed), mode)
> +}
> +
> +fn format_entry_type(entry: &FileEntry, changed: bool) -> String {
> +    let kind = match entry.kind() {
> +        EntryKind::Symlink(_) => "l",
> +        EntryKind::Hardlink(_) => "h",
> +        EntryKind::Device(_) if entry.metadata().stat.is_blockdev() => "b",
> +        EntryKind::Device(_) => "c",
> +        EntryKind::Socket => "s",
> +        EntryKind::Fifo => "p",
> +        EntryKind::File { .. } => "f",
> +        EntryKind::Directory => "d",
> +        _ => " ",
> +    };
> +
> +    format!("{}{}", change_indicator(changed), kind)
> +}
> +
> +fn format_uid(entry: &FileEntry, changed: bool) -> String {
> +    format!("{}{}", change_indicator(changed), entry.metadata().stat.uid)
> +}
> +
> +fn format_gid(entry: &FileEntry, changed: bool) -> String {
> +    format!("{}{}", change_indicator(changed), entry.metadata().stat.gid)
> +}
> +
> +fn format_file_name(entry: &FileEntry, changed: bool) -> String {
> +    format!(
> +        "{}{}",
> +        change_indicator(changed),
> +        entry.file_name().to_string_lossy()
> +    )
> +}
> +
>  /// Display a sorted list of added, modified, deleted files.
>  fn show_file_list(
>      added: &HashMap<&OsStr, &FileEntry>,
>      deleted: &HashMap<&OsStr, &FileEntry>,
> -    modified: &HashMap<&OsStr, &FileEntry>,
> +    modified: &HashMap<&OsStr, (&FileEntry, ChangedProperties)>,
>  ) {
>      let mut all: Vec<&OsStr> = Vec::new();
>  
> @@ -415,28 +532,24 @@ fn show_file_list(
>      all.sort();
>  
>      for file in all {
> -        let (op, entry) = if let Some(entry) = added.get(file) {
> -            ("A", *entry)
> +        let (op, entry, changed) = if let Some(entry) = added.get(file) {
> +            ("A", entry, ChangedProperties::default())
>          } else if let Some(entry) = deleted.get(file) {
> -            ("D", *entry)
> -        } else if let Some(entry) = modified.get(file) {
> -            ("M", *entry)
> +            ("D", entry, ChangedProperties::default())
> +        } else if let Some((entry, changed)) = modified.get(file) {
> +            ("M", entry, *changed)
>          } else {
>              unreachable!();
>          };
>  
> -        let entry_kind = match entry.kind() {
> -            EntryKind::Symlink(_) => "l",
> -            EntryKind::Hardlink(_) => "h",
> -            EntryKind::Device(_) if entry.metadata().stat.is_blockdev() => "b",
> -            EntryKind::Device(_) => "c",
> -            EntryKind::Socket => "s",
> -            EntryKind::Fifo => "p",
> -            EntryKind::File { .. } => "f",
> -            EntryKind::Directory => "d",
> -            _ => " ",
> -        };
> +        let entry_type = format_entry_type(entry, changed.entry_type);
> +        let uid = format_uid(entry, changed.uid);
> +        let gid = format_gid(entry, changed.gid);
> +        let mode = format_mode(entry, changed.mode);
> +        let size = format_filesize(entry, changed.size);
> +        let mtime = format_mtime(entry, changed.mtime);
> +        let name = format_file_name(entry, changed.content);
>  
> -        println!("{} {} {}", op, entry_kind, file.to_string_lossy());
> +        println!("{op} {entry_type:>2} {mode:>5} {uid:>6} {gid:>6} {size:>10} {mtime:11} {name}");
>      }
>  }
> -- 
> 2.30.2

Re: [pbs-devel] [PATCH v3 proxmox-backup 4/4] debug cli: move parameters into the function signature

[Wolfgang Bumiller]

On Wed, Dec 07, 2022 at 10:38:19AM +0100, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> Changes from v2:
>   - This commit is new in v3: `diff_archive_cmd`: Moved parameters into the
>     function signature, instead of manually extracting it from `Value`.
> 
>  src/bin/proxmox_backup_debug/diff.rs | 65 +++++++++++-----------------
>  1 file changed, 25 insertions(+), 40 deletions(-)
> 
> diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
> index 2149720f..3160efb1 100644
> --- a/src/bin/proxmox_backup_debug/diff.rs
> +++ b/src/bin/proxmox_backup_debug/diff.rs
> @@ -25,9 +25,9 @@ use pbs_config::key_config::decrypt_key;
>  use pbs_datastore::dynamic_index::{BufferedDynamicReader, DynamicIndexReader, LocalDynamicReadAt};
>  use pbs_datastore::index::IndexFile;
>  use pbs_tools::crypt_config::CryptConfig;
> -use pbs_tools::json::required_string_param;
>  use pxar::accessor::ReadAt;
>  use pxar::EntryKind;
> +use serde::Deserialize;
>  use serde_json::Value;
>  
>  use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};
> @@ -92,8 +92,7 @@ pub fn diff_commands() -> CommandLineInterface {
>              },
>              "color": {
>                  optional: true,
> -                type: String,
> -                description: "Set mode for colored output. Can be `always`, `auto` or `never`. `auto` will display colors only if stdout is a tty. Defaults to `auto`."
> +                type: ColorMode,
>              }
>          }
>      }
> @@ -102,29 +101,20 @@ pub fn diff_commands() -> CommandLineInterface {
>  /// For modified files, the file metadata (e.g. mode, uid, gid, size, etc.) will be considered. For detecting
>  /// modification of file content, only mtime will be used by default. If the --compare-content flag is provided,
>  /// mtime is ignored and file content will be compared.
> -async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
> +async fn diff_archive_cmd(
> +    prev_snapshot: String,
> +    snapshot: String,
> +    archive_name: String,
> +    compare_content: Option<bool>,

The `#[api]` macro is able to fill in defaults for simple types.

For `compare_content`, you can just drop the `Option<>` and fill in the
`default:` in the `#[api]` macro.

You do want to add `default` to `optional` parameters as much as
possible anyway, since those will be visible in the generated API
documentation.

> +    color: Option<ColorMode>,
> +    ns: Option<BackupNamespace>,

^ These 2 will have to stay an Option for now, since here the API
description is not "visible" to the `#[api]` macro since it cannot
access anything outside its immediate scope.

Basically, whenever `schema: FOO` or `type: Type` is used in `#[api]`,
the `#[api]` macro can do less on its own.
We *could* add the ability to defer to some trait they have to implement
(there's already an `ApiType` trait), but this is not yet implemented
and may be a bit tricky sometimes.

> +    param: Value,
> +) -> Result<(), Error> {
>      let repo = extract_repository_from_value(&param)?;
> -    let snapshot_a = required_string_param(&param, "prev-snapshot")?;
> -    let snapshot_b = required_string_param(&param, "snapshot")?;
> -    let archive_name = required_string_param(&param, "archive-name")?;
> -
> -    let compare_contents = match param.get("compare-content") {
> -        Some(Value::Bool(value)) => *value,
> -        Some(_) => bail!("invalid flag for compare-content"),
> -        None => false,
> -    };
>  
> -    let color = match param.get("color") {
> -        Some(Value::String(color)) => color.as_str().try_into()?,
> -        Some(_) => bail!("invalid color parameter. Valid choices are `always`, `auto` and `never`"),
> -        None => ColorMode::Auto,
> -    };
> -
> -    let namespace = match param.get("ns") {
> -        Some(Value::String(ns)) => ns.parse()?,
> -        Some(_) => bail!("invalid namespace parameter"),
> -        None => BackupNamespace::root(),
> -    };
> +    let compare_content = compare_content.unwrap_or(false);
> +    let color = color.unwrap_or_default();
> +    let namespace = ns.unwrap_or_else(BackupNamespace::root);
>  
>      let crypto = crypto_parameters(&param)?;
>  
> @@ -152,11 +142,11 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
>      if archive_name.ends_with(".pxar") {
>          let file_name = format!("{}.didx", archive_name);
>          diff_archive(
> -            snapshot_a,
> -            snapshot_b,
> +            &prev_snapshot,
> +            &snapshot,
>              &file_name,
>              &repo_params,
> -            compare_contents,
> +            compare_content,
>              &output_params,
>          )
>          .await?;
> @@ -232,25 +222,20 @@ async fn diff_archive(
>      Ok(())
>  }
>  
> +#[api(default: "auto")]
> +#[derive(Default, Deserialize)]
> +#[serde(rename_all = "lowercase")]
> +/// Color output options
>  enum ColorMode {
> +    /// Always output colors
>      Always,
> +    /// Output colors if STDOUT is a tty and neither of TERM=dumb or NO_COLOR is set
> +    #[default]
>      Auto,
> +    /// Never output colors
>      Never,
>  }
>  
> -impl TryFrom<&str> for ColorMode {
> -    type Error = Error;
> -
> -    fn try_from(value: &str) -> Result<Self, Self::Error> {
> -        match value {
> -            "auto" => Ok(Self::Auto),
> -            "always" => Ok(Self::Always),
> -            "never" => Ok(Self::Never),
> -            _ => bail!("invalid color parameter. Valid choices are `always`, `auto` and `never`"),
> -        }
> -    }
> -}
> -
>  struct RepoParams {
>      repo: BackupRepository,
>      crypt_config: Option<Arc<CryptConfig>>,
> -- 
> 2.30.2

Re: [pbs-devel] [PATCH v3 proxmox-backup 1/4] debug cli: show more file attributes for `diff archive` command

[Lukas Wagner]

On 12/9/22 10:13, Wolfgang Bumiller wrote:
>> +        (EntryKind::Device(a), EntryKind::Device(b)) => {
>> +            changed.content = a.major != b.major || a.minor != b.minor
> 
> Just noticed - we might be missing a change between blockdev/chardev
> here? Might need to include
> 
>      file_a.metadata().stat.is_blockdev() != file_b.metadata().stat.is_blockdev()
> 
> as well? Or am I missing this being handled elsewhere already?> 

Yes, you are absolutely correct, this is something that I've missed here. Will be fixed in the next
version of the patch series.

-- 
Best Regards,
Lukas Wagner