* [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(¶m)?;
let snapshot_a = required_string_param(¶m, "prev-snapshot")?;
let snapshot_b = required_string_param(¶m, "snapshot")?;
let archive_name = required_string_param(¶m, "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* 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(¶m)?;
> let snapshot_a = required_string_param(¶m, "prev-snapshot")?;
> let snapshot_b = required_string_param(¶m, "snapshot")?;
> let archive_name = required_string_param(¶m, "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 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
* [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 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