* [pbs-devel] [PATCH v3 proxmox-backup 1/4] debug cli: show more file attributes for `diff archive` command
2022-12-07 9:38 [pbs-devel] [PATCH v3 proxmox-backup 0/4] debug cli: improve output, optionally compare file content for `diff archive` Lukas Wagner
@ 2022-12-07 9:38 ` Lukas Wagner
2022-12-09 9:13 ` Wolfgang Bumiller
2022-12-07 9:38 ` [pbs-devel] [PATCH v3 proxmox-backup 2/4] debug cli: add 'compare-content' flag to " Lukas Wagner
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Lukas Wagner @ 2022-12-07 9:38 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 cb157ec2..d5a4a1fe 100644
--- a/src/bin/proxmox_backup_debug/diff.rs
+++ b/src/bin/proxmox_backup_debug/diff.rs
@@ -11,7 +11,7 @@ use futures::FutureExt;
use proxmox_router::cli::{CliCommand, CliCommandMap, CommandLineInterface};
use proxmox_schema::api;
-use pbs_api_types::{BackupNamespace, BackupPart};
+use pbs_api_types::{BackupNamespace, BackupPart, HumanByte};
use pbs_client::tools::key_source::{
crypto_parameters, format_key_source, get_encryption_key_password, KEYFD_SCHEMA,
};
@@ -173,15 +173,18 @@ async fn diff_archive(
// If file is present in both snapshots, it *might* be modified, but does not have to be.
// If another, unmodified file resides in the same chunk as an actually modified one,
// it will also show up as modified here...
- let potentially_modified: HashMap<&OsStr, &FileEntry> = files_in_a
+ let potentially_modified: HashMap<&OsStr, (&FileEntry, &FileEntry)> = files_in_a
.iter()
- .filter(|(path, _)| files_in_b.contains_key(*path))
- .map(|(path, entry)| (path.as_os_str(), entry))
+ .filter_map(|(path, entry_a)| {
+ files_in_b
+ .get(path)
+ .map(|entry_b| (path.as_os_str(), (entry_a, entry_b)))
+ })
.collect();
// ... so we compare the file metadata/contents to narrow the selection down to files
// which where *really* modified.
- let modified_files = compare_files(&files_in_a, &files_in_b, potentially_modified).await?;
+ let modified_files = compare_files(potentially_modified).await?;
show_file_list(&added_files, &deleted_files, &modified_files);
@@ -335,7 +338,6 @@ fn visit_directory<'f, 'c>(
let digest = chunk_list.get(chunk_index).context("Invalid chunk index")?;
if chunk_diff.get(digest).is_some() {
- // files.insert(file_path.clone().into_os_string());
entries.insert(file_path.into_os_string(), entry);
break;
}
@@ -349,62 +351,177 @@ fn visit_directory<'f, 'c>(
/// Check if files were actually modified
async fn compare_files<'a>(
- entries_a: &HashMap<OsString, FileEntry>,
- entries_b: &HashMap<OsString, FileEntry>,
- files: HashMap<&'a OsStr, &'a FileEntry>,
-) -> Result<HashMap<&'a OsStr, &'a FileEntry>, Error> {
+ files: HashMap<&'a OsStr, (&'a FileEntry, &'a FileEntry)>,
+) -> Result<HashMap<&'a OsStr, (&'a FileEntry, ChangedProperties)>, Error> {
let mut modified_files = HashMap::new();
- for (path, entry) in files {
- let p = path.to_os_string();
- let file_a = entries_a.get(&p).context("File entry not in map")?;
- let file_b = entries_b.get(&p).context("File entry not in map")?;
-
- if !compare_file(file_a, file_b).await {
- modified_files.insert(path, entry);
+ for (path, (entry_a, entry_b)) in files {
+ if let Some(changed) = compare_file(entry_a, entry_b).await? {
+ modified_files.insert(path, (entry_b, changed));
}
}
Ok(modified_files)
}
-async fn compare_file(file_a: &FileEntry, file_b: &FileEntry) -> bool {
- if file_a.metadata() != file_b.metadata() {
- // Check if mtime, permissions, ACLs, etc. have changed - if they have changed, we consider
- // the file as modified.
- return false;
- }
+async fn compare_file(
+ file_a: &FileEntry,
+ file_b: &FileEntry,
+) -> Result<Option<ChangedProperties>, Error> {
+ let mut changed = ChangedProperties::default();
+
+ changed.set_from_metadata(file_a, file_b);
match (file_a.kind(), file_b.kind()) {
(EntryKind::Symlink(a), EntryKind::Symlink(b)) => {
// Check whether the link target has changed.
- a.as_os_str() == b.as_os_str()
+ changed.content = a.as_os_str() != b.as_os_str();
}
(EntryKind::Hardlink(a), EntryKind::Hardlink(b)) => {
// Check whether the link target has changed.
- a.as_os_str() == b.as_os_str()
+ changed.content = a.as_os_str() != b.as_os_str();
+ }
+ (EntryKind::Device(a), EntryKind::Device(b)) => {
+ changed.content = a.major != b.major || a.minor != b.minor
}
- (EntryKind::Device(a), EntryKind::Device(b)) => a.major == b.major && a.minor == b.minor,
- (EntryKind::Socket, EntryKind::Socket) => true,
- (EntryKind::Fifo, EntryKind::Fifo) => true,
(EntryKind::File { size: size_a, .. }, EntryKind::File { size: size_b, .. }) => {
- // At this point we know that all metadata including mtime is
- // the same. To speed things up, we consider the files as equal if they also have
- // the same size.
- // If one were completely paranoid, one could compare the actual file contents,
- // but this decreases performance drastically.
- size_a == size_b
+ if size_a != size_b {
+ changed.size = true;
+ changed.content = true;
+ };
+ }
+ (EntryKind::Directory, EntryKind::Directory) => {}
+ (EntryKind::Socket, EntryKind::Socket) => {}
+ (EntryKind::Fifo, EntryKind::Fifo) => {}
+ (_, _) => {
+ changed.entry_type = true;
}
- (EntryKind::Directory, EntryKind::Directory) => true,
- (_, _) => false, // Kind has changed, so we of course consider it modified.
+ }
+
+ if changed.any() {
+ Ok(Some(changed))
+ } else {
+ Ok(None)
+ }
+}
+
+#[derive(Copy, Clone, Default)]
+struct ChangedProperties {
+ entry_type: bool,
+ mtime: bool,
+ acl: bool,
+ xattrs: bool,
+ fcaps: bool,
+ quota_project_id: bool,
+ mode: bool,
+ flags: bool,
+ uid: bool,
+ gid: bool,
+ size: bool,
+ content: bool,
+}
+
+impl ChangedProperties {
+ fn set_from_metadata(&mut self, file_a: &FileEntry, file_b: &FileEntry) {
+ let a = file_a.metadata();
+ let b = file_b.metadata();
+
+ self.acl = a.acl != b.acl;
+ self.xattrs = a.xattrs != b.xattrs;
+ self.fcaps = a.fcaps != b.fcaps;
+ self.quota_project_id = a.quota_project_id != b.quota_project_id;
+ self.mode = a.stat.mode != b.stat.mode;
+ self.flags = a.stat.flags != b.stat.flags;
+ self.uid = a.stat.uid != b.stat.uid;
+ self.gid = a.stat.gid != b.stat.gid;
+ self.mtime = a.stat.mtime != b.stat.mtime;
+ }
+
+ fn any(&self) -> bool {
+ self.entry_type
+ || self.mtime
+ || self.acl
+ || self.xattrs
+ || self.fcaps
+ || self.quota_project_id
+ || self.mode
+ || self.flags
+ || self.uid
+ || self.gid
+ || self.content
+ }
+}
+
+fn change_indicator(changed: bool) -> &'static str {
+ if changed {
+ "*"
+ } else {
+ " "
+ }
+}
+
+fn format_filesize(entry: &FileEntry, changed: bool) -> String {
+ if let Some(size) = entry.file_size() {
+ format!(
+ "{}{:.1}",
+ change_indicator(changed),
+ HumanByte::new_decimal(size as f64)
+ )
+ } else {
+ String::new()
}
}
+fn format_mtime(entry: &FileEntry, changed: bool) -> String {
+ let mtime = &entry.metadata().stat.mtime;
+
+ let format = if changed { "*%F %T" } else { " %F %T" };
+
+ proxmox_time::strftime_local(format, mtime.secs).unwrap_or_default()
+}
+
+fn format_mode(entry: &FileEntry, changed: bool) -> String {
+ let mode = entry.metadata().stat.mode & 0o7777;
+ format!("{}{:o}", change_indicator(changed), mode)
+}
+
+fn format_entry_type(entry: &FileEntry, changed: bool) -> String {
+ let kind = match entry.kind() {
+ EntryKind::Symlink(_) => "l",
+ EntryKind::Hardlink(_) => "h",
+ EntryKind::Device(_) if entry.metadata().stat.is_blockdev() => "b",
+ EntryKind::Device(_) => "c",
+ EntryKind::Socket => "s",
+ EntryKind::Fifo => "p",
+ EntryKind::File { .. } => "f",
+ EntryKind::Directory => "d",
+ _ => " ",
+ };
+
+ format!("{}{}", change_indicator(changed), kind)
+}
+
+fn format_uid(entry: &FileEntry, changed: bool) -> String {
+ format!("{}{}", change_indicator(changed), entry.metadata().stat.uid)
+}
+
+fn format_gid(entry: &FileEntry, changed: bool) -> String {
+ format!("{}{}", change_indicator(changed), entry.metadata().stat.gid)
+}
+
+fn format_file_name(entry: &FileEntry, changed: bool) -> String {
+ format!(
+ "{}{}",
+ change_indicator(changed),
+ entry.file_name().to_string_lossy()
+ )
+}
+
/// Display a sorted list of added, modified, deleted files.
fn show_file_list(
added: &HashMap<&OsStr, &FileEntry>,
deleted: &HashMap<&OsStr, &FileEntry>,
- modified: &HashMap<&OsStr, &FileEntry>,
+ modified: &HashMap<&OsStr, (&FileEntry, ChangedProperties)>,
) {
let mut all: Vec<&OsStr> = Vec::new();
@@ -415,28 +532,24 @@ fn show_file_list(
all.sort();
for file in all {
- let (op, entry) = if let Some(entry) = added.get(file) {
- ("A", *entry)
+ let (op, entry, changed) = if let Some(entry) = added.get(file) {
+ ("A", entry, ChangedProperties::default())
} else if let Some(entry) = deleted.get(file) {
- ("D", *entry)
- } else if let Some(entry) = modified.get(file) {
- ("M", *entry)
+ ("D", entry, ChangedProperties::default())
+ } else if let Some((entry, changed)) = modified.get(file) {
+ ("M", entry, *changed)
} else {
unreachable!();
};
- let entry_kind = match entry.kind() {
- EntryKind::Symlink(_) => "l",
- EntryKind::Hardlink(_) => "h",
- EntryKind::Device(_) if entry.metadata().stat.is_blockdev() => "b",
- EntryKind::Device(_) => "c",
- EntryKind::Socket => "s",
- EntryKind::Fifo => "p",
- EntryKind::File { .. } => "f",
- EntryKind::Directory => "d",
- _ => " ",
- };
+ let entry_type = format_entry_type(entry, changed.entry_type);
+ let uid = format_uid(entry, changed.uid);
+ let gid = format_gid(entry, changed.gid);
+ let mode = format_mode(entry, changed.mode);
+ let size = format_filesize(entry, changed.size);
+ let mtime = format_mtime(entry, changed.mtime);
+ let name = format_file_name(entry, changed.content);
- println!("{} {} {}", op, entry_kind, file.to_string_lossy());
+ println!("{op} {entry_type:>2} {mode:>5} {uid:>6} {gid:>6} {size:>10} {mtime:11} {name}");
}
}
--
2.30.2
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [pbs-devel] [PATCH v3 proxmox-backup 1/4] debug cli: show more file attributes for `diff archive` command
2022-12-07 9:38 ` [pbs-devel] [PATCH v3 proxmox-backup 1/4] debug cli: show more file attributes for `diff archive` command Lukas Wagner
@ 2022-12-09 9:13 ` Wolfgang Bumiller
2022-12-09 10:45 ` Lukas Wagner
0 siblings, 1 reply; 8+ messages in thread
From: Wolfgang Bumiller @ 2022-12-09 9:13 UTC (permalink / raw)
To: Lukas Wagner; +Cc: pbs-devel
On Wed, Dec 07, 2022 at 10:38:16AM +0100, Lukas Wagner wrote:
> This commit enriches the output of the `diff archive` command,
> showing pxar entry type, mode, uid, gid, size, mtime and filename.
> Attributes that changed between both snapshots are prefixed
> with a "*".
>
> For instance:
>
> $ proxmox-backup-debug diff archive ...
> A f 644 10045 10000 0 B 2022-11-28 13:44:51 add.txt
> M f 644 10045 10000 6 B *2022-11-28 13:45:05 content.txt
> D f 644 10045 10000 0 B 2022-11-28 13:17:09 deleted.txt
> M f 644 10045 *29 0 B 2022-11-28 13:16:20 gid.txt
> M f *777 10045 10000 0 B 2022-11-28 13:42:47 mode.txt
> M f 644 10045 10000 0 B *2022-11-28 13:44:33 mtime.txt
> M f 644 10045 10000 *7 B *2022-11-28 13:44:59 *size.txt
> M f 644 *64045 10000 0 B 2022-11-28 13:16:18 uid.txt
> M *f 644 10045 10000 10 B 2022-11-28 13:44:59 type_changed.txt
>
> Also, this commit ensures that we always show the *new* type.
> Previously, the command showed the old type if it was changed.
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> src/bin/proxmox_backup_debug/diff.rs | 221 ++++++++++++++++++++-------
> 1 file changed, 167 insertions(+), 54 deletions(-)
>
> diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
> index cb157ec2..d5a4a1fe 100644
> --- a/src/bin/proxmox_backup_debug/diff.rs
> +++ b/src/bin/proxmox_backup_debug/diff.rs
> @@ -11,7 +11,7 @@ use futures::FutureExt;
> use proxmox_router::cli::{CliCommand, CliCommandMap, CommandLineInterface};
> use proxmox_schema::api;
>
> -use pbs_api_types::{BackupNamespace, BackupPart};
> +use pbs_api_types::{BackupNamespace, BackupPart, HumanByte};
> use pbs_client::tools::key_source::{
> crypto_parameters, format_key_source, get_encryption_key_password, KEYFD_SCHEMA,
> };
> @@ -173,15 +173,18 @@ async fn diff_archive(
> // If file is present in both snapshots, it *might* be modified, but does not have to be.
> // If another, unmodified file resides in the same chunk as an actually modified one,
> // it will also show up as modified here...
> - let potentially_modified: HashMap<&OsStr, &FileEntry> = files_in_a
> + let potentially_modified: HashMap<&OsStr, (&FileEntry, &FileEntry)> = files_in_a
> .iter()
> - .filter(|(path, _)| files_in_b.contains_key(*path))
> - .map(|(path, entry)| (path.as_os_str(), entry))
> + .filter_map(|(path, entry_a)| {
> + files_in_b
> + .get(path)
> + .map(|entry_b| (path.as_os_str(), (entry_a, entry_b)))
> + })
> .collect();
>
> // ... so we compare the file metadata/contents to narrow the selection down to files
> // which where *really* modified.
> - let modified_files = compare_files(&files_in_a, &files_in_b, potentially_modified).await?;
> + let modified_files = compare_files(potentially_modified).await?;
>
> show_file_list(&added_files, &deleted_files, &modified_files);
>
> @@ -335,7 +338,6 @@ fn visit_directory<'f, 'c>(
> let digest = chunk_list.get(chunk_index).context("Invalid chunk index")?;
>
> if chunk_diff.get(digest).is_some() {
> - // files.insert(file_path.clone().into_os_string());
> entries.insert(file_path.into_os_string(), entry);
> break;
> }
> @@ -349,62 +351,177 @@ fn visit_directory<'f, 'c>(
>
> /// Check if files were actually modified
> async fn compare_files<'a>(
> - entries_a: &HashMap<OsString, FileEntry>,
> - entries_b: &HashMap<OsString, FileEntry>,
> - files: HashMap<&'a OsStr, &'a FileEntry>,
> -) -> Result<HashMap<&'a OsStr, &'a FileEntry>, Error> {
> + files: HashMap<&'a OsStr, (&'a FileEntry, &'a FileEntry)>,
> +) -> Result<HashMap<&'a OsStr, (&'a FileEntry, ChangedProperties)>, Error> {
> let mut modified_files = HashMap::new();
>
> - for (path, entry) in files {
> - let p = path.to_os_string();
> - let file_a = entries_a.get(&p).context("File entry not in map")?;
> - let file_b = entries_b.get(&p).context("File entry not in map")?;
> -
> - if !compare_file(file_a, file_b).await {
> - modified_files.insert(path, entry);
> + for (path, (entry_a, entry_b)) in files {
> + if let Some(changed) = compare_file(entry_a, entry_b).await? {
> + modified_files.insert(path, (entry_b, changed));
> }
> }
>
> Ok(modified_files)
> }
>
> -async fn compare_file(file_a: &FileEntry, file_b: &FileEntry) -> bool {
> - if file_a.metadata() != file_b.metadata() {
> - // Check if mtime, permissions, ACLs, etc. have changed - if they have changed, we consider
> - // the file as modified.
> - return false;
> - }
> +async fn compare_file(
> + file_a: &FileEntry,
> + file_b: &FileEntry,
> +) -> Result<Option<ChangedProperties>, Error> {
> + let mut changed = ChangedProperties::default();
> +
> + changed.set_from_metadata(file_a, file_b);
>
> match (file_a.kind(), file_b.kind()) {
> (EntryKind::Symlink(a), EntryKind::Symlink(b)) => {
> // Check whether the link target has changed.
> - a.as_os_str() == b.as_os_str()
> + changed.content = a.as_os_str() != b.as_os_str();
> }
> (EntryKind::Hardlink(a), EntryKind::Hardlink(b)) => {
> // Check whether the link target has changed.
> - a.as_os_str() == b.as_os_str()
> + changed.content = a.as_os_str() != b.as_os_str();
> + }
> + (EntryKind::Device(a), EntryKind::Device(b)) => {
> + changed.content = a.major != b.major || a.minor != b.minor
Just noticed - we might be missing a change between blockdev/chardev
here? Might need to include
file_a.metadata().stat.is_blockdev() != file_b.metadata().stat.is_blockdev()
as well? Or am I missing this being handled elsewhere already?
> }
> - (EntryKind::Device(a), EntryKind::Device(b)) => a.major == b.major && a.minor == b.minor,
> - (EntryKind::Socket, EntryKind::Socket) => true,
> - (EntryKind::Fifo, EntryKind::Fifo) => true,
> (EntryKind::File { size: size_a, .. }, EntryKind::File { size: size_b, .. }) => {
> - // At this point we know that all metadata including mtime is
> - // the same. To speed things up, we consider the files as equal if they also have
> - // the same size.
> - // If one were completely paranoid, one could compare the actual file contents,
> - // but this decreases performance drastically.
> - size_a == size_b
> + if size_a != size_b {
> + changed.size = true;
> + changed.content = true;
> + };
> + }
> + (EntryKind::Directory, EntryKind::Directory) => {}
> + (EntryKind::Socket, EntryKind::Socket) => {}
> + (EntryKind::Fifo, EntryKind::Fifo) => {}
> + (_, _) => {
> + changed.entry_type = true;
> }
> - (EntryKind::Directory, EntryKind::Directory) => true,
> - (_, _) => false, // Kind has changed, so we of course consider it modified.
> + }
> +
> + if changed.any() {
> + Ok(Some(changed))
> + } else {
> + Ok(None)
> + }
> +}
> +
> +#[derive(Copy, Clone, Default)]
> +struct ChangedProperties {
> + entry_type: bool,
> + mtime: bool,
> + acl: bool,
> + xattrs: bool,
> + fcaps: bool,
> + quota_project_id: bool,
> + mode: bool,
> + flags: bool,
> + uid: bool,
> + gid: bool,
> + size: bool,
> + content: bool,
> +}
> +
> +impl ChangedProperties {
> + fn set_from_metadata(&mut self, file_a: &FileEntry, file_b: &FileEntry) {
> + let a = file_a.metadata();
> + let b = file_b.metadata();
> +
> + self.acl = a.acl != b.acl;
> + self.xattrs = a.xattrs != b.xattrs;
> + self.fcaps = a.fcaps != b.fcaps;
> + self.quota_project_id = a.quota_project_id != b.quota_project_id;
> + self.mode = a.stat.mode != b.stat.mode;
> + self.flags = a.stat.flags != b.stat.flags;
> + self.uid = a.stat.uid != b.stat.uid;
> + self.gid = a.stat.gid != b.stat.gid;
> + self.mtime = a.stat.mtime != b.stat.mtime;
> + }
> +
> + fn any(&self) -> bool {
> + self.entry_type
> + || self.mtime
> + || self.acl
> + || self.xattrs
> + || self.fcaps
> + || self.quota_project_id
> + || self.mode
> + || self.flags
> + || self.uid
> + || self.gid
> + || self.content
> + }
> +}
> +
> +fn change_indicator(changed: bool) -> &'static str {
> + if changed {
> + "*"
> + } else {
> + " "
> + }
> +}
> +
> +fn format_filesize(entry: &FileEntry, changed: bool) -> String {
> + if let Some(size) = entry.file_size() {
> + format!(
> + "{}{:.1}",
> + change_indicator(changed),
> + HumanByte::new_decimal(size as f64)
> + )
> + } else {
> + String::new()
> }
> }
>
> +fn format_mtime(entry: &FileEntry, changed: bool) -> String {
> + let mtime = &entry.metadata().stat.mtime;
> +
> + let format = if changed { "*%F %T" } else { " %F %T" };
> +
> + proxmox_time::strftime_local(format, mtime.secs).unwrap_or_default()
> +}
> +
> +fn format_mode(entry: &FileEntry, changed: bool) -> String {
> + let mode = entry.metadata().stat.mode & 0o7777;
> + format!("{}{:o}", change_indicator(changed), mode)
> +}
> +
> +fn format_entry_type(entry: &FileEntry, changed: bool) -> String {
> + let kind = match entry.kind() {
> + EntryKind::Symlink(_) => "l",
> + EntryKind::Hardlink(_) => "h",
> + EntryKind::Device(_) if entry.metadata().stat.is_blockdev() => "b",
> + EntryKind::Device(_) => "c",
> + EntryKind::Socket => "s",
> + EntryKind::Fifo => "p",
> + EntryKind::File { .. } => "f",
> + EntryKind::Directory => "d",
> + _ => " ",
> + };
> +
> + format!("{}{}", change_indicator(changed), kind)
> +}
> +
> +fn format_uid(entry: &FileEntry, changed: bool) -> String {
> + format!("{}{}", change_indicator(changed), entry.metadata().stat.uid)
> +}
> +
> +fn format_gid(entry: &FileEntry, changed: bool) -> String {
> + format!("{}{}", change_indicator(changed), entry.metadata().stat.gid)
> +}
> +
> +fn format_file_name(entry: &FileEntry, changed: bool) -> String {
> + format!(
> + "{}{}",
> + change_indicator(changed),
> + entry.file_name().to_string_lossy()
> + )
> +}
> +
> /// Display a sorted list of added, modified, deleted files.
> fn show_file_list(
> added: &HashMap<&OsStr, &FileEntry>,
> deleted: &HashMap<&OsStr, &FileEntry>,
> - modified: &HashMap<&OsStr, &FileEntry>,
> + modified: &HashMap<&OsStr, (&FileEntry, ChangedProperties)>,
> ) {
> let mut all: Vec<&OsStr> = Vec::new();
>
> @@ -415,28 +532,24 @@ fn show_file_list(
> all.sort();
>
> for file in all {
> - let (op, entry) = if let Some(entry) = added.get(file) {
> - ("A", *entry)
> + let (op, entry, changed) = if let Some(entry) = added.get(file) {
> + ("A", entry, ChangedProperties::default())
> } else if let Some(entry) = deleted.get(file) {
> - ("D", *entry)
> - } else if let Some(entry) = modified.get(file) {
> - ("M", *entry)
> + ("D", entry, ChangedProperties::default())
> + } else if let Some((entry, changed)) = modified.get(file) {
> + ("M", entry, *changed)
> } else {
> unreachable!();
> };
>
> - let entry_kind = match entry.kind() {
> - EntryKind::Symlink(_) => "l",
> - EntryKind::Hardlink(_) => "h",
> - EntryKind::Device(_) if entry.metadata().stat.is_blockdev() => "b",
> - EntryKind::Device(_) => "c",
> - EntryKind::Socket => "s",
> - EntryKind::Fifo => "p",
> - EntryKind::File { .. } => "f",
> - EntryKind::Directory => "d",
> - _ => " ",
> - };
> + let entry_type = format_entry_type(entry, changed.entry_type);
> + let uid = format_uid(entry, changed.uid);
> + let gid = format_gid(entry, changed.gid);
> + let mode = format_mode(entry, changed.mode);
> + let size = format_filesize(entry, changed.size);
> + let mtime = format_mtime(entry, changed.mtime);
> + let name = format_file_name(entry, changed.content);
>
> - println!("{} {} {}", op, entry_kind, file.to_string_lossy());
> + println!("{op} {entry_type:>2} {mode:>5} {uid:>6} {gid:>6} {size:>10} {mtime:11} {name}");
> }
> }
> --
> 2.30.2
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [pbs-devel] [PATCH v3 proxmox-backup 1/4] debug cli: show more file attributes for `diff archive` command
2022-12-09 9:13 ` Wolfgang Bumiller
@ 2022-12-09 10:45 ` Lukas Wagner
0 siblings, 0 replies; 8+ messages in thread
From: Lukas Wagner @ 2022-12-09 10:45 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: pbs-devel
On 12/9/22 10:13, Wolfgang Bumiller wrote:
>> + (EntryKind::Device(a), EntryKind::Device(b)) => {
>> + changed.content = a.major != b.major || a.minor != b.minor
>
> Just noticed - we might be missing a change between blockdev/chardev
> here? Might need to include
>
> file_a.metadata().stat.is_blockdev() != file_b.metadata().stat.is_blockdev()
>
> as well? Or am I missing this being handled elsewhere already?>
Yes, you are absolutely correct, this is something that I've missed here. Will be fixed in the next
version of the patch series.
--
Best Regards,
Lukas Wagner
^ permalink raw reply [flat|nested] 8+ messages in thread
* [pbs-devel] [PATCH v3 proxmox-backup 2/4] debug cli: add 'compare-content' flag to `diff archive` command
2022-12-07 9:38 [pbs-devel] [PATCH v3 proxmox-backup 0/4] debug cli: improve output, optionally compare file content for `diff archive` Lukas Wagner
2022-12-07 9:38 ` [pbs-devel] [PATCH v3 proxmox-backup 1/4] debug cli: show more file attributes for `diff archive` command Lukas Wagner
@ 2022-12-07 9:38 ` Lukas Wagner
2022-12-07 9:38 ` [pbs-devel] [PATCH v3 proxmox-backup 3/4] debug cli: add colored output for `diff archive` Lukas Wagner
2022-12-07 9:38 ` [pbs-devel] [PATCH v3 proxmox-backup 4/4] debug cli: move parameters into the function signature Lukas Wagner
3 siblings, 0 replies; 8+ messages in thread
From: Lukas Wagner @ 2022-12-07 9:38 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 v2:
- Increase buffersize to 4K
- Added workaround for weird issue with tokio::try_join! and #[api]
src/bin/proxmox_backup_debug/diff.rs | 158 +++++++++++++++++++++++--
src/bin/proxmox_backup_manager/ldap.rs | 102 ++++++++++++++++
2 files changed, 248 insertions(+), 12 deletions(-)
create mode 100644 src/bin/proxmox_backup_manager/ldap.rs
diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
index d5a4a1fe..a8a4e08d 100644
--- a/src/bin/proxmox_backup_debug/diff.rs
+++ b/src/bin/proxmox_backup_debug/diff.rs
@@ -28,12 +28,15 @@ use pbs_tools::json::required_string_param;
use pxar::accessor::ReadAt;
use pxar::EntryKind;
use serde_json::Value;
+use tokio::io::AsyncReadExt;
type ChunkDigest = [u8; 32];
type FileEntry = pxar::accessor::aio::FileEntry<Arc<dyn ReadAt + Send + Sync>>;
type Accessor = pxar::accessor::aio::Accessor<Arc<dyn ReadAt + Send + Sync>>;
type Directory = pxar::accessor::aio::Directory<Arc<dyn ReadAt + Send + Sync>>;
+const BUFFERSIZE: usize = 4096;
+
pub fn diff_commands() -> CommandLineInterface {
let cmd_def = CliCommandMap::new().insert(
"archive",
@@ -79,18 +82,30 @@ pub fn diff_commands() -> CommandLineInterface {
schema: KEYFD_SCHEMA,
optional: true,
},
+ "compare-content": {
+ optional: true,
+ type: bool,
+ description: "Compare file content rather than solely relying on mtime for detecting modified files.",
+ },
}
}
)]
/// Diff an archive in two snapshots. The command will output a list of added, modified and deleted files.
-/// For modified files, only the file metadata (e.g. mtime, size, etc.) will be considered. The actual
-/// file contents will not be compared.
+/// For modified files, the file metadata (e.g. mode, uid, gid, size, etc.) will be considered. For detecting
+/// modification of file content, only mtime will be used by default. If the --compare-content flag is provided,
+/// mtime is ignored and file content will be compared.
async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
let repo = extract_repository_from_value(¶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,45 @@ async fn compare_file(
}
}
+async fn compare_file_contents(file_a: &FileEntry, file_b: &FileEntry) -> Result<bool, Error> {
+ let mut contents_a = file_a.contents().await?;
+ let mut contents_b = file_b.contents().await?;
+
+ compare_readers(&mut contents_a, &mut contents_b).await
+}
+
+async fn compare_readers<T>(reader_a: &mut T, reader_b: &mut T) -> Result<bool, Error>
+where
+ T: AsyncReadExt + Unpin,
+{
+ let mut buf_a = Box::new([0u8; BUFFERSIZE]);
+ let mut buf_b = Box::new([0u8; BUFFERSIZE]);
+
+ loop {
+ // Put the both read calls into their own async blocks, otherwise
+ // tokio::try_join! in combination with our #[api] macro leads to some
+ // weird `higher-order lifetime error`
+ let read_fut_a = async { reader_a.read(buf_a.as_mut_slice()).await };
+ let read_fut_b = async { reader_b.read(buf_b.as_mut_slice()).await };
+
+ let (bytes_read_a, bytes_read_b) = tokio::try_join!(read_fut_a, read_fut_b)?;
+
+ if bytes_read_a != bytes_read_b {
+ return Ok(false);
+ }
+
+ if bytes_read_a == 0 {
+ break;
+ }
+
+ if buf_a[..bytes_read_a] != buf_b[..bytes_read_b] {
+ return Ok(false);
+ }
+ }
+
+ Ok(true)
+}
+
#[derive(Copy, Clone, Default)]
struct ChangedProperties {
entry_type: bool,
@@ -438,8 +514,11 @@ impl ChangedProperties {
}
fn any(&self) -> bool {
+ self.any_without_mtime() || self.mtime
+ }
+
+ fn any_without_mtime(&self) -> bool {
self.entry_type
- || self.mtime
|| self.acl
|| self.xattrs
|| self.fcaps
@@ -475,9 +554,10 @@ fn format_filesize(entry: &FileEntry, changed: bool) -> String {
fn format_mtime(entry: &FileEntry, changed: bool) -> String {
let mtime = &entry.metadata().stat.mtime;
- let format = if changed { "*%F %T" } else { " %F %T" };
+ let mut format = change_indicator(changed).to_owned();
+ format.push_str("%F %T");
- proxmox_time::strftime_local(format, mtime.secs).unwrap_or_default()
+ proxmox_time::strftime_local(&format, mtime.secs).unwrap_or_default()
}
fn format_mode(entry: &FileEntry, changed: bool) -> String {
@@ -553,3 +633,57 @@ fn show_file_list(
println!("{op} {entry_type:>2} {mode:>5} {uid:>6} {gid:>6} {size:>10} {mtime:11} {name}");
}
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ use std::{
+ io::Cursor,
+ pin::Pin,
+ task::{Context, Poll},
+ };
+ use tokio::io::{AsyncRead, ReadBuf};
+
+ struct MockedAsyncReader(Cursor<Vec<u8>>);
+
+ impl AsyncRead for MockedAsyncReader {
+ fn poll_read(
+ self: Pin<&mut Self>,
+ _cx: &mut Context<'_>,
+ read_buf: &mut ReadBuf<'_>,
+ ) -> Poll<std::io::Result<()>> {
+ let mut buf = vec![0u8; 100];
+
+ let res = std::io::Read::read(&mut self.get_mut().0, &mut buf);
+
+ if let Ok(bytes) = res {
+ read_buf.put_slice(&buf[..bytes]);
+ }
+
+ Poll::Ready(res.map(|_| ()))
+ }
+ }
+
+ #[test]
+ fn test_do_compare_file_contents() {
+ fn compare(a: Vec<u8>, b: Vec<u8>) -> Result<bool, Error> {
+ let mut mock_a = MockedAsyncReader(Cursor::new(a));
+ let mut mock_b = MockedAsyncReader(Cursor::new(b));
+
+ proxmox_async::runtime::block_on(compare_readers(&mut mock_a, &mut mock_b))
+ }
+
+ assert!(matches!(compare(vec![0; 15], vec![0; 15]), Ok(true)));
+ assert!(matches!(compare(vec![0; 15], vec![0; 14]), Ok(false)));
+ assert!(matches!(compare(vec![0; 15], vec![1; 15]), Ok(false)));
+
+ let mut buf = vec![1u8; 2 * BUFFERSIZE];
+ buf[BUFFERSIZE] = 0;
+ assert!(matches!(compare(vec![1u8; 2 * BUFFERSIZE], buf), Ok(false)));
+
+ let mut buf = vec![1u8; 2 * BUFFERSIZE];
+ buf[2 * BUFFERSIZE - 1] = 0;
+ assert!(matches!(compare(vec![1u8; 2 * BUFFERSIZE], buf), Ok(false)));
+ }
+}
diff --git a/src/bin/proxmox_backup_manager/ldap.rs b/src/bin/proxmox_backup_manager/ldap.rs
new file mode 100644
index 00000000..a2f10f66
--- /dev/null
+++ b/src/bin/proxmox_backup_manager/ldap.rs
@@ -0,0 +1,102 @@
+use anyhow::Error;
+use serde_json::Value;
+
+use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
+use proxmox_schema::api;
+
+use pbs_api_types::REALM_ID_SCHEMA;
+
+use proxmox_backup::api2;
+
+#[api(
+ input: {
+ properties: {
+ "output-format": {
+ schema: OUTPUT_FORMAT,
+ optional: true,
+ },
+ }
+ }
+)]
+/// List configured LDAP realms
+fn list_ldap_realms(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+ let output_format = get_output_format(¶m);
+
+ let info = &api2::config::access::ldap::API_METHOD_LIST_LDAP_REALMS;
+ let mut data = match info.handler {
+ ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
+ _ => unreachable!(),
+ };
+
+ let options = default_table_format_options()
+ .column(ColumnConfig::new("realm"))
+ // .column(ColumnConfig::new("issuer-url"))
+ .column(ColumnConfig::new("comment"));
+
+ format_and_print_result_full(&mut data, &info.returns, &output_format, &options);
+
+ Ok(Value::Null)
+}
+#[api(
+ input: {
+ properties: {
+ realm: {
+ schema: REALM_ID_SCHEMA,
+ },
+ "output-format": {
+ schema: OUTPUT_FORMAT,
+ optional: true,
+ },
+ }
+ }
+)]
+
+/// Show LDAP realm configuration
+fn show_ldap_realm(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+ let output_format = get_output_format(¶m);
+
+ let info = &api2::config::access::ldap::API_METHOD_READ_LDAP_REALM;
+ let mut data = match info.handler {
+ ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
+ _ => unreachable!(),
+ };
+
+ let options = default_table_format_options();
+ format_and_print_result_full(&mut data, &info.returns, &output_format, &options);
+
+ Ok(Value::Null)
+}
+
+pub fn ldap_commands() -> CommandLineInterface {
+ let cmd_def = CliCommandMap::new()
+ .insert("list", CliCommand::new(&API_METHOD_LIST_LDAP_REALMS))
+ .insert(
+ "show",
+ CliCommand::new(&API_METHOD_SHOW_LDAP_REALM)
+ .arg_param(&["realm"])
+ .completion_cb("realm", pbs_config::domains::complete_ldap_realm_name),
+ )
+ .insert(
+ "create",
+ CliCommand::new(&api2::config::access::ldap::API_METHOD_CREATE_LDAP_REALM)
+ .arg_param(&["realm"])
+ .arg_param(&["realm"])
+ .completion_cb("realm", pbs_config::domains::complete_ldap_realm_name),
+ )
+ .insert(
+ "update",
+ CliCommand::new(&api2::config::access::ldap::API_METHOD_UPDATE_LDAP_REALM)
+ .arg_param(&["realm"])
+ .arg_param(&["realm"])
+ .completion_cb("realm", pbs_config::domains::complete_ldap_realm_name),
+ )
+ .insert(
+ "delete",
+ CliCommand::new(&api2::config::access::ldap::API_METHOD_DELETE_LDAP_REALM)
+ .arg_param(&["realm"])
+ .arg_param(&["realm"])
+ .completion_cb("realm", pbs_config::domains::complete_ldap_realm_name),
+ );
+
+ cmd_def.into()
+}
--
2.30.2
^ permalink raw reply [flat|nested] 8+ messages in thread* [pbs-devel] [PATCH v3 proxmox-backup 3/4] debug cli: add colored output for `diff archive`
2022-12-07 9:38 [pbs-devel] [PATCH v3 proxmox-backup 0/4] debug cli: improve output, optionally compare file content for `diff archive` Lukas Wagner
2022-12-07 9:38 ` [pbs-devel] [PATCH v3 proxmox-backup 1/4] debug cli: show more file attributes for `diff archive` command Lukas Wagner
2022-12-07 9:38 ` [pbs-devel] [PATCH v3 proxmox-backup 2/4] debug cli: add 'compare-content' flag to " Lukas Wagner
@ 2022-12-07 9:38 ` Lukas Wagner
2022-12-07 9:38 ` [pbs-devel] [PATCH v3 proxmox-backup 4/4] debug cli: move parameters into the function signature Lukas Wagner
3 siblings, 0 replies; 8+ messages in thread
From: Lukas Wagner @ 2022-12-07 9:38 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>
---
Changes from v2:
- Drop dependency on `atty` crate
- Remove modificiations to debian/control
Cargo.toml | 1 +
src/bin/proxmox_backup_debug/diff.rs | 371 ++++++++++++++++++++++-----
2 files changed, 306 insertions(+), 66 deletions(-)
diff --git a/Cargo.toml b/Cargo.toml
index 8ee48127..7fac1bfa 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -75,6 +75,7 @@ serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0"
siphasher = "0.3"
syslog = "4.0"
+termcolor = "1.1.2"
tokio = { version = "1.6", features = [ "fs", "io-util", "io-std", "macros", "net", "parking_lot", "process", "rt", "rt-multi-thread", "signal", "time" ] }
tokio-openssl = "0.6.1"
tokio-stream = "0.1.0"
diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
index a8a4e08d..2149720f 100644
--- a/src/bin/proxmox_backup_debug/diff.rs
+++ b/src/bin/proxmox_backup_debug/diff.rs
@@ -1,5 +1,6 @@
use std::collections::{HashMap, HashSet};
use std::ffi::{OsStr, OsString};
+use std::io::Write;
use std::iter::FromIterator;
use std::path::{Path, PathBuf};
use std::sync::Arc;
@@ -28,6 +29,8 @@ use pbs_tools::json::required_string_param;
use pxar::accessor::ReadAt;
use pxar::EntryKind;
use serde_json::Value;
+
+use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};
use tokio::io::AsyncReadExt;
type ChunkDigest = [u8; 32];
@@ -87,6 +90,11 @@ pub fn diff_commands() -> CommandLineInterface {
type: bool,
description: "Compare file content rather than solely relying on mtime for detecting modified files.",
},
+ "color": {
+ optional: true,
+ type: String,
+ description: "Set mode for colored output. Can be `always`, `auto` or `never`. `auto` will display colors only if stdout is a tty. Defaults to `auto`."
+ }
}
}
)]
@@ -106,6 +114,12 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
None => false,
};
+ let color = match param.get("color") {
+ Some(Value::String(color)) => color.as_str().try_into()?,
+ Some(_) => bail!("invalid color parameter. Valid choices are `always`, `auto` and `never`"),
+ None => ColorMode::Auto,
+ };
+
let namespace = match param.get("ns") {
Some(Value::String(ns)) => ns.parse()?,
Some(_) => bail!("invalid namespace parameter"),
@@ -133,6 +147,8 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
namespace,
};
+ let output_params = OutputParams { color };
+
if archive_name.ends_with(".pxar") {
let file_name = format!("{}.didx", archive_name);
diff_archive(
@@ -141,6 +157,7 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
&file_name,
&repo_params,
compare_contents,
+ &output_params,
)
.await?;
} else {
@@ -156,6 +173,7 @@ async fn diff_archive(
file_name: &str,
repo_params: &RepoParams,
compare_contents: bool,
+ output_params: &OutputParams,
) -> Result<(), Error> {
let (index_a, accessor_a) = open_dynamic_index(snapshot_a, file_name, repo_params).await?;
let (index_b, accessor_b) = open_dynamic_index(snapshot_b, file_name, repo_params).await?;
@@ -209,17 +227,40 @@ async fn diff_archive(
// which where *really* modified.
let modified_files = compare_files(potentially_modified, compare_contents).await?;
- show_file_list(&added_files, &deleted_files, &modified_files);
+ show_file_list(&added_files, &deleted_files, &modified_files, output_params)?;
Ok(())
}
+enum ColorMode {
+ Always,
+ Auto,
+ Never,
+}
+
+impl TryFrom<&str> for ColorMode {
+ type Error = Error;
+
+ fn try_from(value: &str) -> Result<Self, Self::Error> {
+ match value {
+ "auto" => Ok(Self::Auto),
+ "always" => Ok(Self::Always),
+ "never" => Ok(Self::Never),
+ _ => bail!("invalid color parameter. Valid choices are `always`, `auto` and `never`"),
+ }
+ }
+}
+
struct RepoParams {
repo: BackupRepository,
crypt_config: Option<Arc<CryptConfig>>,
namespace: BackupNamespace,
}
+struct OutputParams {
+ color: ColorMode,
+}
+
async fn open_dynamic_index(
snapshot: &str,
archive_name: &str,
@@ -531,70 +572,271 @@ impl ChangedProperties {
}
}
-fn change_indicator(changed: bool) -> &'static str {
- if changed {
- "*"
- } else {
- " "
- }
+enum FileOperation {
+ Added,
+ Modified,
+ Deleted,
}
-fn format_filesize(entry: &FileEntry, changed: bool) -> String {
- if let Some(size) = entry.file_size() {
- format!(
- "{}{:.1}",
- change_indicator(changed),
- HumanByte::new_decimal(size as f64)
- )
- } else {
- String::new()
+struct ColumnWidths {
+ operation: usize,
+ entry_type: usize,
+ uid: usize,
+ gid: usize,
+ mode: usize,
+ filesize: usize,
+ mtime: usize,
+}
+
+impl Default for ColumnWidths {
+ fn default() -> Self {
+ Self {
+ operation: 1,
+ entry_type: 2,
+ uid: 6,
+ gid: 6,
+ mode: 6,
+ filesize: 10,
+ mtime: 11,
+ }
}
}
-fn format_mtime(entry: &FileEntry, changed: bool) -> String {
- let mtime = &entry.metadata().stat.mtime;
+struct FileEntryPrinter {
+ stream: StandardStream,
+ column_widths: ColumnWidths,
+ changed_color: Color,
+}
- let mut format = change_indicator(changed).to_owned();
- format.push_str("%F %T");
+impl FileEntryPrinter {
+ pub fn new(output_params: &OutputParams) -> Self {
+ let color_choice = match output_params.color {
+ ColorMode::Always => ColorChoice::Always,
+ ColorMode::Auto => {
+ if unsafe { libc::isatty(1) == 1 } {
+ // Show colors unless `TERM=dumb` or `NO_COLOR` is set.
+ ColorChoice::Auto
+ } else {
+ ColorChoice::Never
+ }
+ }
+ ColorMode::Never => ColorChoice::Never,
+ };
- proxmox_time::strftime_local(&format, mtime.secs).unwrap_or_default()
-}
+ let stdout = StandardStream::stdout(color_choice);
-fn format_mode(entry: &FileEntry, changed: bool) -> String {
- let mode = entry.metadata().stat.mode & 0o7777;
- format!("{}{:o}", change_indicator(changed), mode)
-}
+ Self {
+ stream: stdout,
+ column_widths: ColumnWidths::default(),
+ changed_color: Color::Yellow,
+ }
+ }
-fn format_entry_type(entry: &FileEntry, changed: bool) -> String {
- let kind = match entry.kind() {
- EntryKind::Symlink(_) => "l",
- EntryKind::Hardlink(_) => "h",
- EntryKind::Device(_) if entry.metadata().stat.is_blockdev() => "b",
- EntryKind::Device(_) => "c",
- EntryKind::Socket => "s",
- EntryKind::Fifo => "p",
- EntryKind::File { .. } => "f",
- EntryKind::Directory => "d",
- _ => " ",
- };
+ fn change_indicator(&self, changed: bool) -> &'static str {
+ if changed {
+ "*"
+ } else {
+ " "
+ }
+ }
- format!("{}{}", change_indicator(changed), kind)
-}
+ fn set_color_if_changed(&mut self, changed: bool) -> Result<(), Error> {
+ if changed {
+ self.stream
+ .set_color(ColorSpec::new().set_fg(Some(self.changed_color)))?;
+ }
-fn format_uid(entry: &FileEntry, changed: bool) -> String {
- format!("{}{}", change_indicator(changed), entry.metadata().stat.uid)
-}
+ Ok(())
+ }
-fn format_gid(entry: &FileEntry, changed: bool) -> String {
- format!("{}{}", change_indicator(changed), entry.metadata().stat.gid)
-}
+ fn write_operation(&mut self, op: FileOperation) -> Result<(), Error> {
+ let (text, color) = match op {
+ FileOperation::Added => ("A", Color::Green),
+ FileOperation::Modified => ("M", Color::Yellow),
+ FileOperation::Deleted => ("D", Color::Red),
+ };
-fn format_file_name(entry: &FileEntry, changed: bool) -> String {
- format!(
- "{}{}",
- change_indicator(changed),
- entry.file_name().to_string_lossy()
- )
+ self.stream
+ .set_color(ColorSpec::new().set_fg(Some(color)))?;
+
+ write!(
+ self.stream,
+ "{text:>width$}",
+ width = self.column_widths.operation,
+ )?;
+
+ self.stream.reset()?;
+
+ Ok(())
+ }
+
+ fn write_filesize(&mut self, entry: &FileEntry, changed: bool) -> Result<(), Error> {
+ let output = if let Some(size) = entry.file_size() {
+ format!(
+ "{}{:.1}",
+ self.change_indicator(changed),
+ HumanByte::new_decimal(size as f64)
+ )
+ } else {
+ String::new()
+ };
+
+ self.set_color_if_changed(changed)?;
+ write!(
+ self.stream,
+ "{output:>width$}",
+ width = self.column_widths.filesize,
+ )?;
+ self.stream.reset()?;
+
+ Ok(())
+ }
+
+ fn write_mtime(&mut self, entry: &FileEntry, changed: bool) -> Result<(), Error> {
+ let mtime = &entry.metadata().stat.mtime;
+
+ let mut format = self.change_indicator(changed).to_owned();
+ format.push_str("%F %T");
+
+ let output = proxmox_time::strftime_local(&format, mtime.secs).unwrap_or_default();
+
+ self.set_color_if_changed(changed)?;
+ write!(
+ self.stream,
+ "{output:>width$}",
+ width = self.column_widths.mtime,
+ )?;
+ self.stream.reset()?;
+
+ Ok(())
+ }
+
+ fn write_mode(&mut self, entry: &FileEntry, changed: bool) -> Result<(), Error> {
+ let mode = entry.metadata().stat.mode & 0o7777;
+ let output = format!("{}{:o}", self.change_indicator(changed), mode);
+
+ self.set_color_if_changed(changed)?;
+ write!(
+ self.stream,
+ "{output:>width$}",
+ width = self.column_widths.mode,
+ )?;
+ self.stream.reset()?;
+
+ Ok(())
+ }
+
+ fn write_entry_type(&mut self, entry: &FileEntry, changed: bool) -> Result<(), Error> {
+ let kind = match entry.kind() {
+ EntryKind::Symlink(_) => "l",
+ EntryKind::Hardlink(_) => "h",
+ EntryKind::Device(_) if entry.metadata().stat.is_blockdev() => "b",
+ EntryKind::Device(_) => "c",
+ EntryKind::Socket => "s",
+ EntryKind::Fifo => "p",
+ EntryKind::File { .. } => "f",
+ EntryKind::Directory => "d",
+ _ => " ",
+ };
+
+ let output = format!("{}{}", self.change_indicator(changed), kind);
+
+ self.set_color_if_changed(changed)?;
+ write!(
+ self.stream,
+ "{output:>width$}",
+ width = self.column_widths.entry_type,
+ )?;
+ self.stream.reset()?;
+
+ Ok(())
+ }
+
+ fn write_uid(&mut self, entry: &FileEntry, changed: bool) -> Result<(), Error> {
+ let output = format!(
+ "{}{}",
+ self.change_indicator(changed),
+ entry.metadata().stat.uid
+ );
+
+ self.set_color_if_changed(changed)?;
+ write!(
+ self.stream,
+ "{output:>width$}",
+ width = self.column_widths.uid,
+ )?;
+ self.stream.reset()?;
+ Ok(())
+ }
+
+ fn write_gid(&mut self, entry: &FileEntry, changed: bool) -> Result<(), Error> {
+ let output = format!(
+ "{}{}",
+ self.change_indicator(changed),
+ entry.metadata().stat.gid
+ );
+
+ self.set_color_if_changed(changed)?;
+ write!(
+ self.stream,
+ "{output:>width$}",
+ width = self.column_widths.gid,
+ )?;
+ self.stream.reset()?;
+ Ok(())
+ }
+
+ fn write_file_name(&mut self, entry: &FileEntry, changed: bool) -> Result<(), Error> {
+ self.set_color_if_changed(changed)?;
+ write!(
+ self.stream,
+ "{}{}",
+ self.change_indicator(changed),
+ entry.file_name().to_string_lossy()
+ )?;
+ self.stream.reset()?;
+
+ Ok(())
+ }
+
+ fn write_column_seperator(&mut self) -> Result<(), Error> {
+ write!(self.stream, " ")?;
+ Ok(())
+ }
+
+ /// Print a file entry, including `changed` indicators and column seperators
+ pub fn print_file_entry(
+ &mut self,
+ entry: &FileEntry,
+ changed: &ChangedProperties,
+ operation: FileOperation,
+ ) -> Result<(), Error> {
+ self.write_operation(operation)?;
+ self.write_column_seperator()?;
+
+ self.write_entry_type(entry, changed.entry_type)?;
+ self.write_column_seperator()?;
+
+ self.write_uid(entry, changed.uid)?;
+ self.write_column_seperator()?;
+
+ self.write_gid(entry, changed.gid)?;
+ self.write_column_seperator()?;
+
+ self.write_mode(entry, changed.mode)?;
+ self.write_column_seperator()?;
+
+ self.write_filesize(entry, changed.size)?;
+ self.write_column_seperator()?;
+
+ self.write_mtime(entry, changed.mtime)?;
+ self.write_column_seperator()?;
+
+ self.write_file_name(entry, changed.content)?;
+ writeln!(self.stream)?;
+
+ Ok(())
+ }
}
/// Display a sorted list of added, modified, deleted files.
@@ -602,7 +844,8 @@ fn show_file_list(
added: &HashMap<&OsStr, &FileEntry>,
deleted: &HashMap<&OsStr, &FileEntry>,
modified: &HashMap<&OsStr, (&FileEntry, ChangedProperties)>,
-) {
+ output_params: &OutputParams,
+) -> Result<(), Error> {
let mut all: Vec<&OsStr> = Vec::new();
all.extend(added.keys());
@@ -611,27 +854,23 @@ fn show_file_list(
all.sort();
+ let mut printer = FileEntryPrinter::new(output_params);
+
for file in all {
- let (op, entry, changed) = if let Some(entry) = added.get(file) {
- ("A", entry, ChangedProperties::default())
+ let (operation, entry, changed) = if let Some(entry) = added.get(file) {
+ (FileOperation::Added, entry, ChangedProperties::default())
} else if let Some(entry) = deleted.get(file) {
- ("D", entry, ChangedProperties::default())
+ (FileOperation::Deleted, entry, ChangedProperties::default())
} else if let Some((entry, changed)) = modified.get(file) {
- ("M", entry, *changed)
+ (FileOperation::Modified, entry, *changed)
} else {
unreachable!();
};
- let entry_type = format_entry_type(entry, changed.entry_type);
- let uid = format_uid(entry, changed.uid);
- let gid = format_gid(entry, changed.gid);
- let mode = format_mode(entry, changed.mode);
- let size = format_filesize(entry, changed.size);
- let mtime = format_mtime(entry, changed.mtime);
- let name = format_file_name(entry, changed.content);
-
- println!("{op} {entry_type:>2} {mode:>5} {uid:>6} {gid:>6} {size:>10} {mtime:11} {name}");
+ printer.print_file_entry(entry, &changed, operation)?;
}
+
+ Ok(())
}
#[cfg(test)]
--
2.30.2
^ permalink raw reply [flat|nested] 8+ messages in thread* [pbs-devel] [PATCH v3 proxmox-backup 4/4] debug cli: move parameters into the function signature
2022-12-07 9:38 [pbs-devel] [PATCH v3 proxmox-backup 0/4] debug cli: improve output, optionally compare file content for `diff archive` Lukas Wagner
` (2 preceding siblings ...)
2022-12-07 9:38 ` [pbs-devel] [PATCH v3 proxmox-backup 3/4] debug cli: add colored output for `diff archive` Lukas Wagner
@ 2022-12-07 9:38 ` Lukas Wagner
2022-12-09 9:22 ` Wolfgang Bumiller
3 siblings, 1 reply; 8+ messages in thread
From: Lukas Wagner @ 2022-12-07 9:38 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
Changes from v2:
- This commit is new in v3: `diff_archive_cmd`: Moved parameters into the
function signature, instead of manually extracting it from `Value`.
src/bin/proxmox_backup_debug/diff.rs | 65 +++++++++++-----------------
1 file changed, 25 insertions(+), 40 deletions(-)
diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
index 2149720f..3160efb1 100644
--- a/src/bin/proxmox_backup_debug/diff.rs
+++ b/src/bin/proxmox_backup_debug/diff.rs
@@ -25,9 +25,9 @@ use pbs_config::key_config::decrypt_key;
use pbs_datastore::dynamic_index::{BufferedDynamicReader, DynamicIndexReader, LocalDynamicReadAt};
use pbs_datastore::index::IndexFile;
use pbs_tools::crypt_config::CryptConfig;
-use pbs_tools::json::required_string_param;
use pxar::accessor::ReadAt;
use pxar::EntryKind;
+use serde::Deserialize;
use serde_json::Value;
use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};
@@ -92,8 +92,7 @@ pub fn diff_commands() -> CommandLineInterface {
},
"color": {
optional: true,
- type: String,
- description: "Set mode for colored output. Can be `always`, `auto` or `never`. `auto` will display colors only if stdout is a tty. Defaults to `auto`."
+ type: ColorMode,
}
}
}
@@ -102,29 +101,20 @@ pub fn diff_commands() -> CommandLineInterface {
/// For modified files, the file metadata (e.g. mode, uid, gid, size, etc.) will be considered. For detecting
/// modification of file content, only mtime will be used by default. If the --compare-content flag is provided,
/// mtime is ignored and file content will be compared.
-async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
+async fn diff_archive_cmd(
+ prev_snapshot: String,
+ snapshot: String,
+ archive_name: String,
+ compare_content: Option<bool>,
+ color: Option<ColorMode>,
+ ns: Option<BackupNamespace>,
+ param: Value,
+) -> Result<(), Error> {
let repo = extract_repository_from_value(¶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 color = match param.get("color") {
- Some(Value::String(color)) => color.as_str().try_into()?,
- Some(_) => bail!("invalid color parameter. Valid choices are `always`, `auto` and `never`"),
- None => ColorMode::Auto,
- };
-
- let namespace = match param.get("ns") {
- Some(Value::String(ns)) => ns.parse()?,
- Some(_) => bail!("invalid namespace parameter"),
- None => BackupNamespace::root(),
- };
+ let compare_content = compare_content.unwrap_or(false);
+ let color = color.unwrap_or_default();
+ let namespace = ns.unwrap_or_else(BackupNamespace::root);
let crypto = crypto_parameters(¶m)?;
@@ -152,11 +142,11 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
if archive_name.ends_with(".pxar") {
let file_name = format!("{}.didx", archive_name);
diff_archive(
- snapshot_a,
- snapshot_b,
+ &prev_snapshot,
+ &snapshot,
&file_name,
&repo_params,
- compare_contents,
+ compare_content,
&output_params,
)
.await?;
@@ -232,25 +222,20 @@ async fn diff_archive(
Ok(())
}
+#[api(default: "auto")]
+#[derive(Default, Deserialize)]
+#[serde(rename_all = "lowercase")]
+/// Color output options
enum ColorMode {
+ /// Always output colors
Always,
+ /// Output colors if STDOUT is a tty and neither of TERM=dumb or NO_COLOR is set
+ #[default]
Auto,
+ /// Never output colors
Never,
}
-impl TryFrom<&str> for ColorMode {
- type Error = Error;
-
- fn try_from(value: &str) -> Result<Self, Self::Error> {
- match value {
- "auto" => Ok(Self::Auto),
- "always" => Ok(Self::Always),
- "never" => Ok(Self::Never),
- _ => bail!("invalid color parameter. Valid choices are `always`, `auto` and `never`"),
- }
- }
-}
-
struct RepoParams {
repo: BackupRepository,
crypt_config: Option<Arc<CryptConfig>>,
--
2.30.2
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [pbs-devel] [PATCH v3 proxmox-backup 4/4] debug cli: move parameters into the function signature
2022-12-07 9:38 ` [pbs-devel] [PATCH v3 proxmox-backup 4/4] debug cli: move parameters into the function signature Lukas Wagner
@ 2022-12-09 9:22 ` Wolfgang Bumiller
0 siblings, 0 replies; 8+ messages in thread
From: Wolfgang Bumiller @ 2022-12-09 9:22 UTC (permalink / raw)
To: Lukas Wagner; +Cc: pbs-devel
On Wed, Dec 07, 2022 at 10:38:19AM +0100, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> Changes from v2:
> - This commit is new in v3: `diff_archive_cmd`: Moved parameters into the
> function signature, instead of manually extracting it from `Value`.
>
> src/bin/proxmox_backup_debug/diff.rs | 65 +++++++++++-----------------
> 1 file changed, 25 insertions(+), 40 deletions(-)
>
> diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
> index 2149720f..3160efb1 100644
> --- a/src/bin/proxmox_backup_debug/diff.rs
> +++ b/src/bin/proxmox_backup_debug/diff.rs
> @@ -25,9 +25,9 @@ use pbs_config::key_config::decrypt_key;
> use pbs_datastore::dynamic_index::{BufferedDynamicReader, DynamicIndexReader, LocalDynamicReadAt};
> use pbs_datastore::index::IndexFile;
> use pbs_tools::crypt_config::CryptConfig;
> -use pbs_tools::json::required_string_param;
> use pxar::accessor::ReadAt;
> use pxar::EntryKind;
> +use serde::Deserialize;
> use serde_json::Value;
>
> use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};
> @@ -92,8 +92,7 @@ pub fn diff_commands() -> CommandLineInterface {
> },
> "color": {
> optional: true,
> - type: String,
> - description: "Set mode for colored output. Can be `always`, `auto` or `never`. `auto` will display colors only if stdout is a tty. Defaults to `auto`."
> + type: ColorMode,
> }
> }
> }
> @@ -102,29 +101,20 @@ pub fn diff_commands() -> CommandLineInterface {
> /// For modified files, the file metadata (e.g. mode, uid, gid, size, etc.) will be considered. For detecting
> /// modification of file content, only mtime will be used by default. If the --compare-content flag is provided,
> /// mtime is ignored and file content will be compared.
> -async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
> +async fn diff_archive_cmd(
> + prev_snapshot: String,
> + snapshot: String,
> + archive_name: String,
> + compare_content: Option<bool>,
The `#[api]` macro is able to fill in defaults for simple types.
For `compare_content`, you can just drop the `Option<>` and fill in the
`default:` in the `#[api]` macro.
You do want to add `default` to `optional` parameters as much as
possible anyway, since those will be visible in the generated API
documentation.
> + color: Option<ColorMode>,
> + ns: Option<BackupNamespace>,
^ These 2 will have to stay an Option for now, since here the API
description is not "visible" to the `#[api]` macro since it cannot
access anything outside its immediate scope.
Basically, whenever `schema: FOO` or `type: Type` is used in `#[api]`,
the `#[api]` macro can do less on its own.
We *could* add the ability to defer to some trait they have to implement
(there's already an `ApiType` trait), but this is not yet implemented
and may be a bit tricky sometimes.
> + param: Value,
> +) -> Result<(), Error> {
> let repo = extract_repository_from_value(¶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 color = match param.get("color") {
> - Some(Value::String(color)) => color.as_str().try_into()?,
> - Some(_) => bail!("invalid color parameter. Valid choices are `always`, `auto` and `never`"),
> - None => ColorMode::Auto,
> - };
> -
> - let namespace = match param.get("ns") {
> - Some(Value::String(ns)) => ns.parse()?,
> - Some(_) => bail!("invalid namespace parameter"),
> - None => BackupNamespace::root(),
> - };
> + let compare_content = compare_content.unwrap_or(false);
> + let color = color.unwrap_or_default();
> + let namespace = ns.unwrap_or_else(BackupNamespace::root);
>
> let crypto = crypto_parameters(¶m)?;
>
> @@ -152,11 +142,11 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
> if archive_name.ends_with(".pxar") {
> let file_name = format!("{}.didx", archive_name);
> diff_archive(
> - snapshot_a,
> - snapshot_b,
> + &prev_snapshot,
> + &snapshot,
> &file_name,
> &repo_params,
> - compare_contents,
> + compare_content,
> &output_params,
> )
> .await?;
> @@ -232,25 +222,20 @@ async fn diff_archive(
> Ok(())
> }
>
> +#[api(default: "auto")]
> +#[derive(Default, Deserialize)]
> +#[serde(rename_all = "lowercase")]
> +/// Color output options
> enum ColorMode {
> + /// Always output colors
> Always,
> + /// Output colors if STDOUT is a tty and neither of TERM=dumb or NO_COLOR is set
> + #[default]
> Auto,
> + /// Never output colors
> Never,
> }
>
> -impl TryFrom<&str> for ColorMode {
> - type Error = Error;
> -
> - fn try_from(value: &str) -> Result<Self, Self::Error> {
> - match value {
> - "auto" => Ok(Self::Auto),
> - "always" => Ok(Self::Always),
> - "never" => Ok(Self::Never),
> - _ => bail!("invalid color parameter. Valid choices are `always`, `auto` and `never`"),
> - }
> - }
> -}
> -
> struct RepoParams {
> repo: BackupRepository,
> crypt_config: Option<Arc<CryptConfig>>,
> --
> 2.30.2
^ permalink raw reply [flat|nested] 8+ messages in thread