[pbs-devel] [PATCH proxmox] compression: zip_directory: improve error handling

[pbs-devel] [PATCH proxmox] compression: zip_directory: improve error handling

[Dominik Csapak]

when zipping a directory, our intention was to skip over files that
cannot be zipped (e.g. the file can't be read/vanished/etc.), so we
ignored errors and simply logged it.

but when 'add_entry' fails, we will never actually restore, since every
error there is fatal to the point that the zip cannot be finished thats
because we take the 'target' sink out of self, and only insert it again
after all writes succeeded. so if an error occurs in between 'target' is
not put into self again (and never will be) and the zip cannot be
finished (even if we would catch all those intermediate errors and
restore 'target', we don't know in which state the output was, so we're
unable to finish a valid zip)

to fix that, split the actual 'add_entry' part there out of the async
move block and treat its errors always as fatal

without this, we generate heaps of log lines even after an error
occurred, and can never recover

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
note that i know we use 'eprintln' instead of the log macro, but it's
not part of the fix, and i don't know if we want to add the log crate as
dep here. we only use this function currently in the restore daemon for
downloading a directory

 proxmox-compression/src/zip.rs | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/proxmox-compression/src/zip.rs b/proxmox-compression/src/zip.rs
index c9ed80e..6d457fb 100644
--- a/proxmox-compression/src/zip.rs
+++ b/proxmox-compression/src/zip.rs
@@ -643,7 +643,7 @@ where
         let entry_path = entry.path().to_owned();
         let encoder = &mut encoder;
 
-        if let Err(err) = async move {
+        match async move {
             let entry_path_no_base = entry.path().strip_prefix(base_path)?;
             let metadata = entry.metadata()?;
             let mtime = match metadata
@@ -659,22 +659,27 @@ where
             if entry.file_type().is_file() {
                 let file = tokio::fs::File::open(entry.path()).await?;
                 let ze = ZipEntry::new(entry_path_no_base, mtime, mode, true);
-                encoder.add_entry(ze, Some(file)).await?;
+                Ok(Some((ze, Some(file))))
             } else if entry.file_type().is_dir() {
                 let ze = ZipEntry::new(entry_path_no_base, mtime, mode, false);
                 let content: Option<tokio::fs::File> = None;
-                encoder.add_entry(ze, content).await?;
+                Ok(Some((ze, content)))
+            } else {
+                // ignore other file types
+                Ok::<_, Error>(None)
             }
-            // ignore other file types
-            Ok::<(), Error>(())
         }
         .await
         {
-            eprintln!(
-                "zip: error encoding file or directory '{}': {}",
-                entry_path.display(),
-                err
-            );
+            Ok(Some((ze, content))) => encoder.add_entry(ze, content).await?,
+            Ok(None) => {}
+            Err(err) => {
+                eprintln!(
+                    "zip: error encoding file or directory '{}': {}",
+                    entry_path.display(),
+                    err
+                );
+            }
         }
     }
 
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox] compression: zip_directory: improve error handling

[Dominik Csapak]

ping?

would be nice if we could get this in, so that we can ship a
proxmox-backup-file-restore package with that included for
pve 8

[pbs-devel] applied: [PATCH proxmox] compression: zip_directory: improve error handling

[Wolfgang Bumiller]

applied

but that style... the match operand is too large...
fixed that up in a followup commit

On Wed, Jun 14, 2023 at 11:58:27AM +0200, Dominik Csapak wrote:
> ping?
> 
> would be nice if we could get this in, so that we can ship a
> proxmox-backup-file-restore package with that included for
> pve 8