From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <d.csapak@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with UTF8SMTPS id BA617617B8
 for <pbs-devel@lists.proxmox.com>; Wed, 21 Oct 2020 12:39:39 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with UTF8SMTP id AB7341741B
 for <pbs-devel@lists.proxmox.com>; Wed, 21 Oct 2020 12:39:39 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [212.186.127.180])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with UTF8SMTPS id 4C31B1740C
 for <pbs-devel@lists.proxmox.com>; Wed, 21 Oct 2020 12:39:38 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with UTF8SMTP id 0DEA945EA1
 for <pbs-devel@lists.proxmox.com>; Wed, 21 Oct 2020 12:39:38 +0200 (CEST)
To: pbs-devel@lists.proxmox.com
References: <20201021100501.24286-1-d.csapak@proxmox.com>
From: Dominik Csapak <d.csapak@proxmox.com>
Message-ID: <83145525-f6d4-df02-a8b6-f31e8e0be3de@proxmox.com>
Date: Wed, 21 Oct 2020 12:39:33 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:82.0) Gecko/20100101
 Thunderbird/82.0
MIME-Version: 1.0
In-Reply-To: <20201021100501.24286-1-d.csapak@proxmox.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.232 Adjusted score from AWL reputation of From: address
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 KAM_NUMSUBJECT 0.5 Subject ends in numbers excluding current years
 NICE_REPLY_A           -0.001 Looks like a legit reply (A)
 RCVD_IN_DNSWL_MED        -2.3 Sender listed at https://www.dnswl.org/,
 medium trust
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
 http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
 information. [create.rs, catalog.rs]
Subject: Re: [pbs-devel] [PATCH proxmox-backup] fix #2998: encode mtime as
 i64 instead of u64
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
 <pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Wed, 21 Oct 2020 10:39:39 -0000

On 10/21/20 12:05 PM, Dominik Csapak wrote:
> saves files mtime as i64 instead of u64 which enables backup of
> files with negative mtime
> 
> the catalog_decode_i64 is compatible to encoded u64 values
> but not reverse, so all "old" catalogs can be read with the new
> decoder, but catalogs that contain negative mtimes will decode wrongly
> on older clients
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>   src/backup/catalog.rs | 76 +++++++++++++++++++++++++++++++++++++++----
>   src/pxar/catalog.rs   |  2 +-
>   src/pxar/create.rs    |  2 +-
>   3 files changed, 72 insertions(+), 8 deletions(-)
> 
> diff --git a/src/backup/catalog.rs b/src/backup/catalog.rs
> index d1f519e5..dd892a88 100644
> --- a/src/backup/catalog.rs
> +++ b/src/backup/catalog.rs
> @@ -78,7 +78,7 @@ pub struct DirEntry {
>   #[derive(Clone, Debug, PartialEq)]
>   pub enum DirEntryAttribute {
>       Directory { start: u64 },
> -    File { size: u64, mtime: u64 },
> +    File { size: u64, mtime: i64 },
>       Symlink,
>       Hardlink,
>       BlockDevice,
> @@ -89,7 +89,7 @@ pub enum DirEntryAttribute {
>   
>   impl DirEntry {
>   
> -    fn new(etype: CatalogEntryType, name: Vec<u8>, start: u64, size: u64, mtime:u64) -> Self {
> +    fn new(etype: CatalogEntryType, name: Vec<u8>, start: u64, size: u64, mtime: i64) -> Self {
>           match etype {
>               CatalogEntryType::Directory => {
>                   DirEntry { name, attr: DirEntryAttribute::Directory { start } }
> @@ -184,7 +184,7 @@ impl DirInfo {
>                   catalog_encode_u64(writer, name.len() as u64)?;
>                   writer.write_all(name)?;
>                   catalog_encode_u64(writer, *size)?;
> -                catalog_encode_u64(writer, *mtime)?;
> +                catalog_encode_i64(writer, *mtime)?;
>               }
>               DirEntry { name, attr: DirEntryAttribute::Symlink } => {
>                   writer.write_all(&[CatalogEntryType::Symlink as u8])?;
> @@ -234,7 +234,7 @@ impl DirInfo {
>           Ok((self.name, data))
>       }
>   
> -    fn parse<C: FnMut(CatalogEntryType, &[u8], u64, u64, u64) -> Result<bool, Error>>(
> +    fn parse<C: FnMut(CatalogEntryType, &[u8], u64, u64, i64) -> Result<bool, Error>>(
>           data: &[u8],
>           mut callback: C,
>       ) -> Result<(), Error> {
> @@ -265,7 +265,7 @@ impl DirInfo {
>                   }
>                   CatalogEntryType::File => {
>                       let size = catalog_decode_u64(&mut cursor)?;
> -                    let mtime = catalog_decode_u64(&mut cursor)?;
> +                    let mtime = catalog_decode_i64(&mut cursor)?;
>                       callback(etype, name, 0, size, mtime)?
>                   }
>                   _ => {
> @@ -362,7 +362,7 @@ impl <W: Write> BackupCatalogWriter for CatalogWriter<W> {
>           Ok(())
>       }
>   
> -    fn add_file(&mut self, name: &CStr, size: u64, mtime: u64) -> Result<(), Error> {
> +    fn add_file(&mut self, name: &CStr, size: u64, mtime: i64) -> Result<(), Error> {
>           let dir = self.dirstack.last_mut().ok_or_else(|| format_err!("outside root"))?;
>           let name = name.to_bytes().to_vec();
>           dir.entries.push(DirEntry { name, attr: DirEntryAttribute::File { size, mtime } });
> @@ -587,6 +587,70 @@ impl <R: Read + Seek> CatalogReader<R> {
>       }
>   }
>   
> +/// Serialize i64 as short, variable length byte sequence
> +///
> +/// Stores 7 bits per byte, Bit 8 indicates the end of the sequence (when not set).
> +/// We limit values to a maximum of 2^63.
> +/// If the first byte is 128 (all zero but the end marker), the number is negative

typo here: s/first/last/ and 0x0 instead of 128
i can send a v2 or a follow-up

> +pub fn catalog_encode_i64<W: Write>(writer: &mut W, v: i64) -> Result<(), Error> {
> +    let mut enc = Vec::new();
> +
> +    let mut d = if v < 0 {
> +        (-1 * v) as u64
> +    } else {
> +        v as u64
> +    };
> +
> +    loop {
> +        if d < 128 {
> +            if v < 0 {
> +                enc.push(128 | d as u8);
> +                enc.push(0u8);
> +            } else {
> +                enc.push(d as u8);
> +            }
> +            break;
> +        }
> +        enc.push((128 | (d & 127)) as u8);
> +        d = d >> 7;
> +    }
> +    writer.write_all(&enc)?;
> +
> +    Ok(())
> +}
> +
> +/// Deserialize i64 from variable length byte sequence
> +///
> +/// We currently read maximal 10 bytes, which give a maximum of 63 bits + sign.
> +/// this method is compatible with catalog_encode_u64, meaning it can
> +/// properly decode values that are encoded with catalog_encode_u64
> +/// (which are <2^63)
> +pub fn catalog_decode_i64<R: Read>(reader: &mut R) -> Result<i64, Error> {
> +
> +    let mut v: u64 = 0;
> +    let mut buf = [0u8];
> +
> +    for i in 0..10 { // only allow 10 bytes (63 bits + sign marker)
> +        if buf.is_empty() {
> +            bail!("decode_i64 failed - unexpected EOB");
> +        }
> +        reader.read_exact(&mut buf)?;
> +
> +        let t = buf[0];
> +
> +        if t == 0 {
> +            return Ok(v as i64 * -1);
> +        } else if t < 128 {
> +            v |= (t as u64) << (i*7);
> +            return Ok(v as i64);
> +        } else {
> +            v |= ((t & 127) as u64) << (i*7);
> +        }
> +    }
> +
> +    bail!("decode_i64 failed - missing end marker");
> +}
> +
>   /// Serialize u64 as short, variable length byte sequence
>   ///
>   /// Stores 7 bits per byte, Bit 8 indicates the end of the sequence (when not set).
> diff --git a/src/pxar/catalog.rs b/src/pxar/catalog.rs
> index 96b42427..02b57dfa 100644
> --- a/src/pxar/catalog.rs
> +++ b/src/pxar/catalog.rs
> @@ -9,7 +9,7 @@ use std::ffi::CStr;
>   pub trait BackupCatalogWriter {
>       fn start_directory(&mut self, name: &CStr) -> Result<(), Error>;
>       fn end_directory(&mut self) -> Result<(), Error>;
> -    fn add_file(&mut self, name: &CStr, size: u64, mtime: u64) -> Result<(), Error>;
> +    fn add_file(&mut self, name: &CStr, size: u64, mtime: i64) -> Result<(), Error>;
>       fn add_symlink(&mut self, name: &CStr) -> Result<(), Error>;
>       fn add_hardlink(&mut self, name: &CStr) -> Result<(), Error>;
>       fn add_block_device(&mut self, name: &CStr) -> Result<(), Error>;
> diff --git a/src/pxar/create.rs b/src/pxar/create.rs
> index 74c49050..6ba6f15c 100644
> --- a/src/pxar/create.rs
> +++ b/src/pxar/create.rs
> @@ -535,7 +535,7 @@ impl<'a, 'b> Archiver<'a, 'b> {
>   
>                   let file_size = stat.st_size as u64;
>                   if let Some(ref mut catalog) = self.catalog {
> -                    catalog.add_file(c_file_name, file_size, stat.st_mtime as u64)?;
> +                    catalog.add_file(c_file_name, file_size, stat.st_mtime)?;
>                   }
>   
>                   let offset: LinkOffset =
>