public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH v2 proxmox-backup 0/3] debug cli: improve output, optionally compare file content for `diff archive`
@ 2022-12-05 14:55 Lukas Wagner
  2022-12-05 14:55 ` [pbs-devel] [PATCH v2 proxmox-backup 1/3] debug cli: show more file attributes for `diff archive` command Lukas Wagner
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Lukas Wagner @ 2022-12-05 14:55 UTC (permalink / raw)
  To: pbs-devel

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 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 (3):
  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 to `diff archive` command

 Cargo.toml                           |   2 +
 debian/control                       |   2 +
 src/bin/proxmox_backup_debug/diff.rs | 604 ++++++++++++++++++++++++---
 3 files changed, 549 insertions(+), 59 deletions(-)

-- 
2.30.2





^ permalink raw reply	[flat|nested] 8+ messages in thread

* [pbs-devel] [PATCH v2 proxmox-backup 1/3] debug cli: show more file attributes for `diff archive` command
  2022-12-05 14:55 [pbs-devel] [PATCH v2 proxmox-backup 0/3] debug cli: improve output, optionally compare file content for `diff archive` Lukas Wagner
@ 2022-12-05 14:55 ` Lukas Wagner
  2022-12-05 14:55 ` [pbs-devel] [PATCH v2 proxmox-backup 2/3] debug cli: add 'compare-content' flag to " Lukas Wagner
  2022-12-05 14:55 ` [pbs-devel] [PATCH v2 proxmox-backup 3/3] debug cli: add colored output " Lukas Wagner
  2 siblings, 0 replies; 8+ messages in thread
From: Lukas Wagner @ 2022-12-05 14:55 UTC (permalink / raw)
  To: pbs-devel

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 c304c110..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





^ permalink raw reply	[flat|nested] 8+ messages in thread

* [pbs-devel] [PATCH v2 proxmox-backup 2/3] debug cli: add 'compare-content' flag to `diff archive` command
  2022-12-05 14:55 [pbs-devel] [PATCH v2 proxmox-backup 0/3] debug cli: improve output, optionally compare file content for `diff archive` Lukas Wagner
  2022-12-05 14:55 ` [pbs-devel] [PATCH v2 proxmox-backup 1/3] debug cli: show more file attributes for `diff archive` command Lukas Wagner
@ 2022-12-05 14:55 ` Lukas Wagner
  2022-12-06 11:39   ` Wolfgang Bumiller
  2022-12-05 14:55 ` [pbs-devel] [PATCH v2 proxmox-backup 3/3] debug cli: add colored output " Lukas Wagner
  2 siblings, 1 reply; 8+ messages in thread
From: Lukas Wagner @ 2022-12-05 14:55 UTC (permalink / raw)
  To: pbs-devel

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 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

 src/bin/proxmox_backup_debug/diff.rs | 157 +++++++++++++++++++++++++--
 1 file changed, 145 insertions(+), 12 deletions(-)

diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
index d5a4a1fe..b5ecd721 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 = 1024;
+
 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,44 @@ 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 = vec![0u8; BUFFERSIZE];
+    let mut buf_b = vec![0u8; BUFFERSIZE];
+
+    loop {
+        // normally, a join! would be nice here, but this leads to
+        // some 'higher-order lifetime error' for which I'm not smart enough to solve.
+        // let (bytes_read_a, bytes_read_b) =
+        //     tokio::try_join!(reader_a.read(&mut buf_a), reader_b.read(&mut buf_b))?;
+        let bytes_read_a = reader_a.read(&mut buf_a).await?;
+        let bytes_read_b = reader_b.read(&mut buf_b).await?;
+
+        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 +513,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 +553,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 +632,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)));
+    }
+}
-- 
2.30.2





^ permalink raw reply	[flat|nested] 8+ messages in thread

* [pbs-devel] [PATCH v2 proxmox-backup 3/3] debug cli: add colored output to `diff archive` command
  2022-12-05 14:55 [pbs-devel] [PATCH v2 proxmox-backup 0/3] debug cli: improve output, optionally compare file content for `diff archive` Lukas Wagner
  2022-12-05 14:55 ` [pbs-devel] [PATCH v2 proxmox-backup 1/3] debug cli: show more file attributes for `diff archive` command Lukas Wagner
  2022-12-05 14:55 ` [pbs-devel] [PATCH v2 proxmox-backup 2/3] debug cli: add 'compare-content' flag to " Lukas Wagner
@ 2022-12-05 14:55 ` Lukas Wagner
  2022-12-06 11:49   ` Wolfgang Bumiller
  2 siblings, 1 reply; 8+ messages in thread
From: Lukas Wagner @ 2022-12-05 14:55 UTC (permalink / raw)
  To: pbs-devel

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>
---
 Cargo.toml                           |   2 +
 debian/control                       |   2 +
 src/bin/proxmox_backup_debug/diff.rs | 372 ++++++++++++++++++++++-----
 3 files changed, 310 insertions(+), 66 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 38e9c1f2..516dab37 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -42,6 +42,7 @@ path = "src/lib.rs"
 
 [dependencies]
 apt-pkg-native = "0.3.2"
+atty = "0.2.14"
 base64 = "0.13"
 bitflags = "1.2.1"
 bytes = "1.0"
@@ -73,6 +74,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/debian/control b/debian/control
index 6207d85e..3a27cb62 100644
--- a/debian/control
+++ b/debian/control
@@ -7,6 +7,7 @@ Build-Depends: debhelper (>= 12),
  rustc:native,
  libstd-rust-dev,
  librust-anyhow-1+default-dev,
+ librust-atty-0.2.14-dev ,
  librust-apt-pkg-native-0.3+default-dev (>= 0.3.2-~~),
  librust-base64-0.13+default-dev,
  librust-bitflags-1+default-dev (>= 1.2.1-~~),
@@ -94,6 +95,7 @@ Build-Depends: debhelper (>= 12),
  librust-siphasher-0.3+default-dev,
  librust-syslog-4+default-dev,
  librust-tar-0.4+default-dev,
+ librust-termcolor-1.1.2-dev,
  librust-thiserror-1+default-dev,
  librust-tokio-1+default-dev (>= 1.6-~~),
  librust-tokio-1+fs-dev (>= 1.6-~~),
diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
index b5ecd721..d2406ef5 100644
--- a/src/bin/proxmox_backup_debug/diff.rs
+++ b/src/bin/proxmox_backup_debug/diff.rs
@@ -1,5 +1,7 @@
 use std::collections::{HashMap, HashSet};
+use std::convert::{TryFrom, TryInto};
 use std::ffi::{OsStr, OsString};
+use std::io::Write;
 use std::iter::FromIterator;
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
@@ -28,6 +30,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 +91,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 +115,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 +148,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 +158,7 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
             &file_name,
             &repo_params,
             compare_contents,
+            &output_params,
         )
         .await?;
     } else {
@@ -156,6 +174,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 +228,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,
@@ -530,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 atty::is(atty::Stream::Stdout) {
+                    // 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.
@@ -601,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());
@@ -610,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





^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pbs-devel] [PATCH v2 proxmox-backup 2/3] debug cli: add 'compare-content' flag to `diff archive` command
  2022-12-05 14:55 ` [pbs-devel] [PATCH v2 proxmox-backup 2/3] debug cli: add 'compare-content' flag to " Lukas Wagner
@ 2022-12-06 11:39   ` Wolfgang Bumiller
  2022-12-06 14:44     ` Lukas Wagner
  0 siblings, 1 reply; 8+ messages in thread
From: Wolfgang Bumiller @ 2022-12-06 11:39 UTC (permalink / raw)
  To: Lukas Wagner; +Cc: pbs-devel

On Mon, Dec 05, 2022 at 03:55:13PM +0100, Lukas Wagner wrote:
> 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 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
> 
>  src/bin/proxmox_backup_debug/diff.rs | 157 +++++++++++++++++++++++++--
>  1 file changed, 145 insertions(+), 12 deletions(-)
> 
> diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
> index d5a4a1fe..b5ecd721 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 = 1024;

Please bump this ito 4k. Full page sizes have the potential to perform
better (even though in this case we'll rarely ever be page aligned, but it
won't hurt...)

> +
>  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's start to move parameters into the function signature here.
Should work fine for most of them.

The `#[api]` macro does support mixed parameters as well so you can
still use the `extract_repository_from_value` helper. Eg. you could do:

    async fn diff_archive_cmd(
        prev_snapshot: String,
        snapshot: String,
        archive_name: String,
        param: Value,
    ) -> Result<(), Error> {

Though you'll still need to have `let` bindings for the "a" and "b"
renaming, but that'll be easy & clear to read.

> +
>      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,44 @@ 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 = vec![0u8; BUFFERSIZE];
> +    let mut buf_b = vec![0u8; BUFFERSIZE];

Maybe use a Box instead of a Vec, we don't ever resize this atm.

> +
> +    loop {
> +        // normally, a join! would be nice here, but this leads to
> +        // some 'higher-order lifetime error' for which I'm not smart enough to solve.
> +        // let (bytes_read_a, bytes_read_b) =
> +        //     tokio::try_join!(reader_a.read(&mut buf_a), reader_b.read(&mut buf_b))?;

The compile error also isn't useful at all.

Given that all it takes to make it work is to wrap the futures in an
async {} block, I'd call this a compiler bug. If you manage to find a
minimal reproducer w/o requiring pbs code, you could report it.

This one works:

    let (bytes_read_a, bytes_read_b) =
        tokio::try_join!(
            async { reader_a.read(&mut buf_a).await },
            async { reader_b.read(&mut buf_b).await },
        )?;

Note that your commented-out code itself does in fact compile fine, and
can even be used "normally", only when reached *some* way through an
#[api] fn does it error, which probably comes from the use of the
`ApiHandler` enum...

> +        let bytes_read_a = reader_a.read(&mut buf_a).await?;
> +        let bytes_read_b = reader_b.read(&mut buf_b).await?;
> +
> +        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 +513,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 +553,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 +632,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)));
> +    }
> +}
> -- 
> 2.30.2




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pbs-devel] [PATCH v2 proxmox-backup 3/3] debug cli: add colored output to `diff archive` command
  2022-12-05 14:55 ` [pbs-devel] [PATCH v2 proxmox-backup 3/3] debug cli: add colored output " Lukas Wagner
@ 2022-12-06 11:49   ` Wolfgang Bumiller
  0 siblings, 0 replies; 8+ messages in thread
From: Wolfgang Bumiller @ 2022-12-06 11:49 UTC (permalink / raw)
  To: Lukas Wagner; +Cc: pbs-devel

On Mon, Dec 05, 2022 at 03:55:14PM +0100, Lukas Wagner wrote:
> 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>
> ---
>  Cargo.toml                           |   2 +
>  debian/control                       |   2 +
>  src/bin/proxmox_backup_debug/diff.rs | 372 ++++++++++++++++++++++-----
>  3 files changed, 310 insertions(+), 66 deletions(-)
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index 38e9c1f2..516dab37 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -42,6 +42,7 @@ path = "src/lib.rs"
>  
>  [dependencies]
>  apt-pkg-native = "0.3.2"
> +atty = "0.2.14"

Please drop this, it's kind of a weird crate. It should just take an
`&impl AsRawFd`, the enum thing doesn't provide any convenience and it's
just... `libc::isatty(1) == 1` for us...

>  base64 = "0.13"
>  bitflags = "1.2.1"
>  bytes = "1.0"
> @@ -73,6 +74,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/debian/control b/debian/control
> index 6207d85e..3a27cb62 100644
> --- a/debian/control
> +++ b/debian/control

There's generally no need to bump d/control in patch series like this.
I'm auto-generating those ;-)

> @@ -7,6 +7,7 @@ Build-Depends: debhelper (>= 12),
>   rustc:native,
>   libstd-rust-dev,
>   librust-anyhow-1+default-dev,
> + librust-atty-0.2.14-dev ,
>   librust-apt-pkg-native-0.3+default-dev (>= 0.3.2-~~),
>   librust-base64-0.13+default-dev,
>   librust-bitflags-1+default-dev (>= 1.2.1-~~),
> @@ -94,6 +95,7 @@ Build-Depends: debhelper (>= 12),
>   librust-siphasher-0.3+default-dev,
>   librust-syslog-4+default-dev,
>   librust-tar-0.4+default-dev,
> + librust-termcolor-1.1.2-dev,
>   librust-thiserror-1+default-dev,
>   librust-tokio-1+default-dev (>= 1.6-~~),
>   librust-tokio-1+fs-dev (>= 1.6-~~),
> diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
> index b5ecd721..d2406ef5 100644
> --- a/src/bin/proxmox_backup_debug/diff.rs
> +++ b/src/bin/proxmox_backup_debug/diff.rs
> @@ -1,5 +1,7 @@
>  use std::collections::{HashMap, HashSet};
> +use std::convert::{TryFrom, TryInto};
>  use std::ffi::{OsStr, OsString};
> +use std::io::Write;
>  use std::iter::FromIterator;
>  use std::path::{Path, PathBuf};
>  use std::sync::Arc;
> @@ -28,6 +30,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 +91,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`."
> +            }

So for this I'd recommend just using

    type: ColorMode,

Make the 'fn take a

    color: Option<ColorMode>,

parameter...

>          }
>      }
>  )]
> @@ -106,6 +115,12 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
>          None => false,
>      };
>  
> +    let color = match param.get("color") {

... and let this just be: let color = color.unwrap_or_default();

> +        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 +148,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 +158,7 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
>              &file_name,
>              &repo_params,
>              compare_contents,
> +            &output_params,
>          )
>          .await?;
>      } else {
> @@ -156,6 +174,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 +228,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(())
>  }
>  

You can turn this into an API type:

Add:

    #[api(default: "auto")]

(Yes, you still need `default` here even when using #[default], but the
api macro definitely should learn about the #[default] macrker.)

Put your `description` from above here as doc comment.

Add:
    #[derive(Default, Deserialize)]
    #[serde(rename_all = "lowercase")]

> +enum ColorMode {

Add doc comments to the variants

> +    Always,

Add `#[default]` here.

> +    Auto,
> +    Never,
> +}
> +
> +impl TryFrom<&str> for ColorMode {

^ This can be dropped completely then.
Also, for the future, I recommend implementing `FromStr` over `TryFrom`.

> +    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,
> @@ -530,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 atty::is(atty::Stream::Stdout) {

^
Just use unsafe { libc::isatty(1) == 1 }

I know, "unsafe", booh, but that's exactly what it does ;-)

> +                    // 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,
> +        }
> +    }
>  
(...)




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pbs-devel] [PATCH v2 proxmox-backup 2/3] debug cli: add 'compare-content' flag to `diff archive` command
  2022-12-06 11:39   ` Wolfgang Bumiller
@ 2022-12-06 14:44     ` Lukas Wagner
  2022-12-06 15:01       ` Wolfgang Bumiller
  0 siblings, 1 reply; 8+ messages in thread
From: Lukas Wagner @ 2022-12-06 14:44 UTC (permalink / raw)
  To: Wolfgang Bumiller; +Cc: pbs-devel

Hi,

On 12/6/22 12:39, Wolfgang Bumiller wrote:
>> +{
>> +    let mut buf_a = vec![0u8; BUFFERSIZE];
>> +    let mut buf_b = vec![0u8; BUFFERSIZE];
> 
> Maybe use a Box instead of a Vec, we don't ever resize this atm.
> 

Do you mean like this?
   let mut buf_a = Box::new([0u8; BUFFERSIZE]);

AFAIU this would construct the array on the stack first before it is moved into the Box,
leading to some unnecessary memcpys. Or am I missing something here?





^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pbs-devel] [PATCH v2 proxmox-backup 2/3] debug cli: add 'compare-content' flag to `diff archive` command
  2022-12-06 14:44     ` Lukas Wagner
@ 2022-12-06 15:01       ` Wolfgang Bumiller
  0 siblings, 0 replies; 8+ messages in thread
From: Wolfgang Bumiller @ 2022-12-06 15:01 UTC (permalink / raw)
  To: Lukas Wagner; +Cc: pbs-devel

On Tue, Dec 06, 2022 at 03:44:07PM +0100, Lukas Wagner wrote:
> Hi,
> 
> On 12/6/22 12:39, Wolfgang Bumiller wrote:
> > > +{
> > > +    let mut buf_a = vec![0u8; BUFFERSIZE];
> > > +    let mut buf_b = vec![0u8; BUFFERSIZE];
> > 
> > Maybe use a Box instead of a Vec, we don't ever resize this atm.
> > 
> 
> Do you mean like this?
>   let mut buf_a = Box::new([0u8; BUFFERSIZE]);
> 
> AFAIU this would construct the array on the stack first before it is moved into the Box,
> leading to some unnecessary memcpys. Or am I missing something here?

With these sizes - and this is only on the CLI AFAICT - this is not an
issue, and in --release builds this will be optimized out.
I prefer not to do things weirdly because "the compiler does stupid
things" if it can be avoided. (Some kind of "placement-new"-like
construct is *really* missing in rust...)
And none of `Box::new_zeroed*`, `Box::new_uninit*` are stable yet, and
even if they were, there's no ergonomic POD version that doesn't imply a
`MaybeUninit`.

Perhaps we should start a proxmox_bytes crate where we move
proxmox_io::vec to, and add a box module for box::zeroed() and
box::uninit()...




^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-12-06 15:01 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-05 14:55 [pbs-devel] [PATCH v2 proxmox-backup 0/3] debug cli: improve output, optionally compare file content for `diff archive` Lukas Wagner
2022-12-05 14:55 ` [pbs-devel] [PATCH v2 proxmox-backup 1/3] debug cli: show more file attributes for `diff archive` command Lukas Wagner
2022-12-05 14:55 ` [pbs-devel] [PATCH v2 proxmox-backup 2/3] debug cli: add 'compare-content' flag to " Lukas Wagner
2022-12-06 11:39   ` Wolfgang Bumiller
2022-12-06 14:44     ` Lukas Wagner
2022-12-06 15:01       ` Wolfgang Bumiller
2022-12-05 14:55 ` [pbs-devel] [PATCH v2 proxmox-backup 3/3] debug cli: add colored output " Lukas Wagner
2022-12-06 11:49   ` Wolfgang Bumiller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal