[pbs-devel] [PATCH proxmox-backup 1/2] pxar: add file name to path_info when applying metadata

[pbs-devel] [PATCH proxmox-backup 1/2] pxar: add file name to path_info when applying metadata

[Fabian Grünbichler]

else, error messages using this path_info refer to the parent directory instead
of the actual file entry causing the problem. since this is just for
informational purposes, lossy conversion is acceptable.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    noticed while preparing patch #2, can/should maybe be applied even if that one isn't ;)

 pbs-client/src/pxar/extract.rs  | 10 ++++++++--
 pbs-client/src/pxar/metadata.rs |  8 +++++++-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs
index b1245c5fc..c0a1db05d 100644
--- a/pbs-client/src/pxar/extract.rs
+++ b/pbs-client/src/pxar/extract.rs
@@ -724,7 +724,10 @@ impl Extractor {
             self.feature_flags,
             metadata,
             file.as_raw_fd(),
-            self.dir_stack.path(),
+            &self
+                .dir_stack
+                .path()
+                .join(file_name.to_string_lossy().to_string()),
             &mut self.on_error,
         )
     }
@@ -783,7 +786,10 @@ impl Extractor {
             self.feature_flags,
             metadata,
             file.as_raw_fd(),
-            self.dir_stack.path(),
+            &self
+                .dir_stack
+                .path()
+                .join(file_name.to_string_lossy().to_string()),
             &mut self.on_error,
         )
     }
diff --git a/pbs-client/src/pxar/metadata.rs b/pbs-client/src/pxar/metadata.rs
index 8e7a14312..071547094 100644
--- a/pbs-client/src/pxar/metadata.rs
+++ b/pbs-client/src/pxar/metadata.rs
@@ -72,7 +72,13 @@ pub fn apply_at(
         Mode::empty(),
     )?;
 
-    apply(flags, metadata, fd.as_raw_fd(), path_info, on_error)
+    apply(
+        flags,
+        metadata,
+        fd.as_raw_fd(),
+        &path_info.join(file_name.to_string_lossy().to_string()),
+        on_error,
+    )
 }
 
 pub fn apply_initial_flags(
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [RFC proxmox-backup 2/2] pxar: extract: make invalid ACLs non-fatal

[Fabian Grünbichler]

these can occur in practice, and neither setting nor getting them throws an
error. if "invalid" ACLs are non-restorable, this means that creating a pxar
archive with such an ACL is possible, but restoring it isn't.

reported in our community forum:
https://forum.proxmox.com/threads/155477

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    we could also forbid creation of course, but since other tools might create
    such ACLs, this would just reduce what we can backup in practice.. and
    doesn't solve the issue for users that have such backups..
    
    another alternative approach would be to detect and handle certain kinds of
    invalidity, e.g., with multiple entries for a single uid/gid, we could drop all
    but the most restrictive one, and require the resulting ACL to still pass `acl_valid`.

 pbs-client/src/pxar/metadata.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/pbs-client/src/pxar/metadata.rs b/pbs-client/src/pxar/metadata.rs
index 071547094..ad6332157 100644
--- a/pbs-client/src/pxar/metadata.rs
+++ b/pbs-client/src/pxar/metadata.rs
@@ -2,7 +2,7 @@ use std::ffi::{CStr, CString};
 use std::os::unix::io::{AsRawFd, RawFd};
 use std::path::Path;
 
-use anyhow::{anyhow, bail, Context, Error};
+use anyhow::{anyhow, Context, Error};
 use nix::errno::Errno;
 use nix::fcntl::OFlag;
 use nix::sys::stat::Mode;
@@ -300,7 +300,7 @@ fn apply_acls(
     }
 
     if !acl.is_valid() {
-        bail!("Error while restoring ACL - ACL invalid");
+        log::warn!("Warning: {path_info:?} - ACL invalid, attempting restore anyway..");
     }
 
     acl.set_file(c_proc_path, acl::ACL_TYPE_ACCESS)?;
@@ -329,7 +329,7 @@ fn apply_acls(
         }
 
         if !acl.is_valid() {
-            bail!("Error while restoring ACL - ACL invalid");
+            log::warn!("Warning: {path_info:?} - ACL invalid, attempting restore anyway..");
         }
 
         acl.set_file(c_proc_path, acl::ACL_TYPE_DEFAULT)?;
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [RFC proxmox-backup 2/2] pxar: extract: make invalid ACLs non-fatal

[Gabriel Goller]

On 08.10.2024 10:33, Fabian Grünbichler wrote:
>these can occur in practice, and neither setting nor getting them throws an
>error. if "invalid" ACLs are non-restorable, this means that creating a pxar
>archive with such an ACL is possible, but restoring it isn't.
>
>reported in our community forum:
>https://forum.proxmox.com/threads/155477
>
>Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>---
>
>Notes:
>    we could also forbid creation of course, but since other tools might create
>    such ACLs, this would just reduce what we can backup in practice.. and
>    doesn't solve the issue for users that have such backups..

If we go this way, then we could implement something like the
`--skip-e2big-xattr` option.

>    another alternative approach would be to detect and handle certain kinds of
>    invalidity, e.g., with multiple entries for a single uid/gid, we could drop all
>    but the most restrictive one, and require the resulting ACL to still pass `acl_valid`.

We could make it quite 'correct' by also merging entries (when duplicate
user/groups appear) and add empty user/group/other entries if none are
existing [0].

But I don't think it's quite worth it tbh. This approach looks fine to
me.

> pbs-client/src/pxar/metadata.rs | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)

Patch is trivial, but nonetheless consider:

Tested-by: Gabriel Goller <g.goller@proxmox.com>

[0]: https://man7.org/linux/man-pages/man3/acl_valid.3.html


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 1/2] pxar: add file name to path_info when applying metadata

[Thomas Lamprecht]

Am 08.10.24 um 10:33 schrieb Fabian Grünbichler:
> else, error messages using this path_info refer to the parent directory instead
> of the actual file entry causing the problem. since this is just for
> informational purposes, lossy conversion is acceptable.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>     noticed while preparing patch #2, can/should maybe be applied even if that one isn't ;)
> 

For both patches:

Acked-by: Thomas Lamprecht <t.lamprecht@proxmox.com>


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] applied-series: [PATCH proxmox-backup 1/2] pxar: add file name to path_info when applying metadata

[Fabian Grünbichler]

with git trailers folded in, thanks!

Quoting Fabian Grünbichler (2024-10-08 10:33:54)
> else, error messages using this path_info refer to the parent directory instead
> of the actual file entry causing the problem. since this is just for
> informational purposes, lossy conversion is acceptable.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>     noticed while preparing patch #2, can/should maybe be applied even if that one isn't ;)
> 
>  pbs-client/src/pxar/extract.rs  | 10 ++++++++--
>  pbs-client/src/pxar/metadata.rs |  8 +++++++-
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs
> index b1245c5fc..c0a1db05d 100644
> --- a/pbs-client/src/pxar/extract.rs
> +++ b/pbs-client/src/pxar/extract.rs
> @@ -724,7 +724,10 @@ impl Extractor {
>              self.feature_flags,
>              metadata,
>              file.as_raw_fd(),
> -            self.dir_stack.path(),
> +            &self
> +                .dir_stack
> +                .path()
> +                .join(file_name.to_string_lossy().to_string()),
>              &mut self.on_error,
>          )
>      }
> @@ -783,7 +786,10 @@ impl Extractor {
>              self.feature_flags,
>              metadata,
>              file.as_raw_fd(),
> -            self.dir_stack.path(),
> +            &self
> +                .dir_stack
> +                .path()
> +                .join(file_name.to_string_lossy().to_string()),
>              &mut self.on_error,
>          )
>      }
> diff --git a/pbs-client/src/pxar/metadata.rs b/pbs-client/src/pxar/metadata.rs
> index 8e7a14312..071547094 100644
> --- a/pbs-client/src/pxar/metadata.rs
> +++ b/pbs-client/src/pxar/metadata.rs
> @@ -72,7 +72,13 @@ pub fn apply_at(
>          Mode::empty(),
>      )?;
>  
> -    apply(flags, metadata, fd.as_raw_fd(), path_info, on_error)
> +    apply(
> +        flags,
> +        metadata,
> +        fd.as_raw_fd(),
> +        &path_info.join(file_name.to_string_lossy().to_string()),
> +        on_error,
> +    )
>  }
>  
>  pub fn apply_initial_flags(
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel