Re: [pbs-devel] [PATCH proxmox 1/1] fix #4995: compression: Include symlinks in zip file restore

[Christian Ebner <c.ebner@proxmox.com>]

Does not compile, see comments inline.
> On 20.11.2023 12:59 CET Filip Schauer <f.schauer@proxmox.com> wrote:
> 
>  
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  proxmox-compression/src/zip.rs | 46 ++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/proxmox-compression/src/zip.rs b/proxmox-compression/src/zip.rs
> index d2d3fd8..89b079b 100644
> --- a/proxmox-compression/src/zip.rs
> +++ b/proxmox-compression/src/zip.rs
> @@ -204,6 +204,7 @@ pub struct ZipEntry {
>      offset: u64,
>      is_file: bool,
>      is_utf8_filename: bool,
> +    symlink_target: Option<OsString>,
>  }
>  
>  impl ZipEntry {
> @@ -211,7 +212,13 @@ impl ZipEntry {
>      ///
>      /// if is_file is false the path will contain an trailing separator,
>      /// so that the zip file understands that it is a directory
> -    pub fn new<P: AsRef<Path>>(path: P, mtime: i64, mode: u16, is_file: bool) -> Self {
> +    pub fn new<P: AsRef<Path>>(
> +        path: P,
> +        mtime: i64,
> +        mode: u16,
> +        is_file: bool,
> +        symlink_target: Option<&Path>,

You can use `P` instead of the Path reference here, to be consistent with `path`. This will require you to take it `as_ref()` below.

> +    ) -> Self {
>          let mut relpath = PathBuf::new();
>  
>          for comp in path.as_ref().components() {
> @@ -226,6 +233,7 @@ impl ZipEntry {
>  
>          let filename: OsString = relpath.into();
>          let is_utf8_filename = filename.to_str().is_some();
> +        let symlink_target_osstr =  symlink_target.map(|x| x.into());

As mentioned above, when of type `P`, take x.as_ref().into() to get the OsString

>  
>          Self {
>              filename,
> @@ -237,6 +245,7 @@ impl ZipEntry {
>              offset: 0,
>              is_file,
>              is_utf8_filename,
> +            symlink_target: symlink_target_osstr,
>          }
>      }
>  
> @@ -360,7 +369,9 @@ impl ZipEntry {
>                  comment_len: 0,
>                  start_disk: 0,
>                  internal_flags: 0,
> -                external_flags: (self.mode as u32) << 16 | (!self.is_file as u32) << 4,
> +                external_flags: (self.mode as u32) << 16
> +                    | (self.symlink_target.is_some() as u32) << 5,

Will not compile like this, the colon has to be after the next line.

> +                    | (!self.is_file as u32) << 4
>                  offset,
>              },
>          )
> @@ -486,23 +497,30 @@ impl<W: AsyncWrite + Unpin> ZipEncoder<W> {
>              .ok_or_else(|| format_err!("had no target during add entry"))?;
>          entry.offset = self.byte_count.try_into()?;
>          self.byte_count += entry.write_local_header(&mut target).await?;
> -        if let Some(content) = content {
> -            let mut reader = HashWrapper::new(content);
> +
> +        if entry.symlink_target.is_some() || content.is_some() {

I would check for content.is_some() first, as it is more likely to have a regular file as opposed to as symlink

>              let mut enc = DeflateEncoder::with_quality(target, Level::Fastest);
>  
> -            enc.compress(&mut reader).await?;
> +            if let Some(symlink_target) = entry.symlink_target.as_ref() {
> +                let cursor = std::io::Cursor::new(symlink_target.as_bytes());
> +                let mut reader = HashWrapper::new(cursor);
> +                enc.compress(&mut reader).await?;
> +                entry.crc32 = reader.finish().0;
> +            } else if let Some(content) = content {

Same, check for content before checking for symlink

> +                let mut reader = HashWrapper::new(content);
> +                enc.compress(&mut reader).await?;
> +                entry.crc32 = reader.finish().0;
> +            }
> +
>              let total_in = enc.total_in();
>              let total_out = enc.total_out();
>              target = enc.into_inner();
>  
> -            let (crc32, _reader) = reader.finish();
> -
>              self.byte_count += total_out as usize;
>              entry.compressed_size = total_out;
>              entry.uncompressed_size = total_in;
> -
> -            entry.crc32 = crc32;
>          }
> +
>          self.byte_count += entry.write_data_descriptor(&mut target).await?;
>          self.target = Some(target);
>  
> @@ -658,10 +676,16 @@ 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);
> +                let ze = ZipEntry::new(entry_path_no_base, mtime, mode, true, None);
>                  Ok(Some((ze, Some(file))))
>              } else if entry.file_type().is_dir() {
> -                let ze = ZipEntry::new(entry_path_no_base, mtime, mode, false);
> +                let ze = ZipEntry::new(entry_path_no_base, mtime, mode, false, None);
> +                let content: Option<tokio::fs::File> = None;
> +                Ok(Some((ze, content)))
> +            } else if entry.file_type().is_symlink() {
> +                let target = std::fs::read_link(entry.path())?;
> +                let ze =
> +                    ZipEntry::new(entry_path_no_base, mtime, mode, true, Some(target.as_ref()));
>                  let content: Option<tokio::fs::File> = None;
>                  Ok(Some((ze, content)))
>              } else {
> -- 
> 2.39.2