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 ESMTPS id 25F0262B79
 for <pbs-devel@lists.proxmox.com>; Wed, 28 Oct 2020 10:58:34 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 1BF931D895
 for <pbs-devel@lists.proxmox.com>; Wed, 28 Oct 2020 10:58:04 +0100 (CET)
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) server-digest SHA256)
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS id 82DE91D87F
 for <pbs-devel@lists.proxmox.com>; Wed, 28 Oct 2020 10:58:02 +0100 (CET)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 4BA5845F72
 for <pbs-devel@lists.proxmox.com>; Wed, 28 Oct 2020 10:58:02 +0100 (CET)
From: Dominik Csapak <d.csapak@proxmox.com>
To: pbs-devel@lists.proxmox.com
Date: Wed, 28 Oct 2020 10:58:00 +0100
Message-Id: <20201028095801.1737-1-d.csapak@proxmox.com>
X-Mailer: git-send-email 2.20.1
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.447 Adjusted score from AWL reputation of From: address
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 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. [logrotate.rs]
Subject: [pbs-devel] [PATCH proxmox-backup 1/2] tools/logrotate: fix
 compression logic
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, 28 Oct 2020 09:58:34 -0000

we never actually compressed any files, since we only looked at
the extension:
* if it was 'zst' (which was always true for newly rotated files), we
  would not compress it
* even if it was not 'zst', we compressed it inplace, never adding '.zst'
  (possibly compressing them multiple times as zstd)

now we add new rotated files simply as '.X' and add a 'target' to the
compress fn, where we rename it to (but now we have to unlink the source
path)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/tools/logrotate.rs | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/src/tools/logrotate.rs b/src/tools/logrotate.rs
index 0a8203a9..13c73a76 100644
--- a/src/tools/logrotate.rs
+++ b/src/tools/logrotate.rs
@@ -46,9 +46,9 @@ impl LogRotate {
         }
     }
 
-    fn compress(file: &PathBuf, options: &CreateOptions) -> Result<(), Error> {
-        let mut source = File::open(file)?;
-        let (fd, tmp_path) = make_tmp_file(file, options.clone())?;
+    fn compress(source_path: &PathBuf, target_path: &PathBuf, options: &CreateOptions) -> Result<(), Error> {
+        let mut source = File::open(source_path)?;
+        let (fd, tmp_path) = make_tmp_file(target_path, options.clone())?;
         let target = unsafe { File::from_raw_fd(fd) };
         let mut encoder = match zstd::stream::write::Encoder::new(target, 0) {
             Ok(encoder) => encoder,
@@ -60,18 +60,23 @@ impl LogRotate {
 
         if let Err(err) = std::io::copy(&mut source, &mut encoder) {
             let _ = unistd::unlink(&tmp_path);
-            bail!("zstd encoding failed for file {:?} - {}", file, err);
+            bail!("zstd encoding failed for file {:?} - {}", target_path, err);
         }
 
         if let Err(err) = encoder.finish() {
             let _ = unistd::unlink(&tmp_path);
-            bail!("zstd finish failed for file {:?} - {}", file, err);
+            bail!("zstd finish failed for file {:?} - {}", target_path, err);
         }
 
-        if let Err(err) = rename(&tmp_path, file) {
+        if let Err(err) = rename(&tmp_path, target_path) {
             let _ = unistd::unlink(&tmp_path);
-            bail!("rename failed for file {:?} - {}", file, err);
+            bail!("rename failed for file {:?} - {}", target_path, err);
         }
+
+        if let Err(err) = unistd::unlink(source_path) {
+            bail!("unlink failed for file {:?} - {}", source_path, err);
+        }
+
         Ok(())
     }
 
@@ -80,9 +85,8 @@ impl LogRotate {
     ///
     /// e.g. rotates
     /// foo.2.zst => foo.3.zst
-    /// foo.1.zst => foo.2.zst
-    /// foo       => foo.1.zst
-    ///           => foo
+    /// foo.1     => foo.2.zst
+    /// foo       => foo.1
     pub fn do_rotate(&mut self, options: CreateOptions, max_files: Option<usize>) -> Result<(), Error> {
         let mut filenames: Vec<PathBuf> = self.file_names().collect();
         if filenames.is_empty() {
@@ -90,12 +94,7 @@ impl LogRotate {
         }
 
         let mut next_filename = self.base_path.clone().canonicalize()?.into_os_string();
-
-        if self.compress {
-            next_filename.push(format!(".{}.zst", filenames.len()));
-        } else {
-            next_filename.push(format!(".{}", filenames.len()));
-        }
+        next_filename.push(format!(".{}", filenames.len()));
 
         filenames.push(PathBuf::from(next_filename));
         let count = filenames.len();
@@ -105,9 +104,11 @@ impl LogRotate {
         }
 
         if self.compress {
-            for i in 2..count-1 {
+            for i in 2..count {
                 if filenames[i].extension().unwrap_or(std::ffi::OsStr::new("")) != "zst" {
-                    Self::compress(&filenames[i], &options)?;
+                    let mut target = filenames[i].clone().into_os_string();
+                    target.push(".zstd");
+                    Self::compress(&filenames[i], &target.into(), &options)?;
                 }
             }
         }
-- 
2.20.1