From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 CF2BF92151 for ; Thu, 1 Feb 2024 11:23:50 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id AED4ED139 for ; Thu, 1 Feb 2024 11:23:50 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (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 ESMTPS for ; Thu, 1 Feb 2024 11:23:49 +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 DF2D941E13 for ; Thu, 1 Feb 2024 11:23:48 +0100 (CET) Message-ID: <7853d791-8bea-407a-bb80-2d3a6fcfad13@proxmox.com> Date: Thu, 1 Feb 2024 11:23:47 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: pbs-devel@lists.proxmox.com References: <20240131105315.237684-1-m.sandoval@proxmox.com> From: Philipp Hufnagl In-Reply-To: <20240131105315.237684-1-m.sandoval@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 2 AWL -0.089 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy FSL_BULK_SIG 1.93 Bulk signature with no Unsubscribe KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RAZOR2_CF_RANGE_51_100 1.886 Razor2 gives confidence level above 50% RAZOR2_CHECK 0.922 Listed in Razor2 (http://razor.sf.net/) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [mod.rs, github.io, catalog.rs] URIBL_SBL_A 0.1 Contains URL's A record listed in the Spamhaus SBL blocklist [185.199.111.153, 185.199.109.153, 185.199.110.153, 185.199.108.153] Subject: Re: [pbs-devel] [PATCH backup v2] fix-3699: pbs-client/tools: use xdg basedirectories for tmp files X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 01 Feb 2024 10:23:50 -0000 Hello On 1/31/24 11:53, Maximiliano Sandoval wrote: > Adds a helper to create temporal files in XDG_CACHE_HOME. If the we > cannot use that path, we fallback to /tmp as before. > > Signed-off-by: Maximiliano Sandoval > --- > Differences from v1: > - temporal file -> temporary file > - add a test > > pbs-client/src/backup_reader.rs | 31 ++++++------------- > pbs-client/src/backup_writer.rs | 13 ++------ > pbs-client/src/tools/mod.rs | 45 ++++++++++++++++++++++++++++ > proxmox-backup-client/src/catalog.rs | 19 ++---------- > 4 files changed, 59 insertions(+), 49 deletions(-) > > diff --git a/pbs-client/src/backup_reader.rs b/pbs-client/src/backup_reader.rs > index 36d8ebcf..6483f5b2 100644 > --- a/pbs-client/src/backup_reader.rs > +++ b/pbs-client/src/backup_reader.rs > @@ -1,7 +1,6 @@ > use anyhow::{format_err, Error}; > use std::fs::File; > use std::io::{Seek, SeekFrom, Write}; > -use std::os::unix::fs::OpenOptionsExt; > use std::sync::Arc; > > use futures::future::AbortHandle; > @@ -141,18 +140,14 @@ impl BackupReader { > > /// Download a .blob file > /// > - /// This creates a temporary file in /tmp (using O_TMPFILE). The data is verified using > - /// the provided manifest. > + /// This creates a temporary file (using O_TMPFILE). The data is verified > + /// using the provided manifest. > pub async fn download_blob( > &self, > manifest: &BackupManifest, > name: &str, > ) -> Result, Error> { > - let mut tmpfile = std::fs::OpenOptions::new() > - .write(true) > - .read(true) > - .custom_flags(libc::O_TMPFILE) > - .open("/tmp")?; > + let mut tmpfile = crate::tools::create_tmp_file()?; > > self.download(name, &mut tmpfile).await?; > > @@ -167,18 +162,14 @@ impl BackupReader { > > /// Download dynamic index file > /// > - /// This creates a temporary file in /tmp (using O_TMPFILE). The index is verified using > - /// the provided manifest. > + /// This creates a temporary file (using O_TMPFILE). The index is verified > + /// using the provided manifest. > pub async fn download_dynamic_index( > &self, > manifest: &BackupManifest, > name: &str, > ) -> Result { > - let mut tmpfile = std::fs::OpenOptions::new() > - .write(true) > - .read(true) > - .custom_flags(libc::O_TMPFILE) > - .open("/tmp")?; > + let mut tmpfile = crate::tools::create_tmp_file()?; > > self.download(name, &mut tmpfile).await?; > > @@ -194,18 +185,14 @@ impl BackupReader { > > /// Download fixed index file > /// > - /// This creates a temporary file in /tmp (using O_TMPFILE). The index is verified using > - /// the provided manifest. > + /// This creates a temporary file (using O_TMPFILE). The index is verified > + /// using the provided manifest. > pub async fn download_fixed_index( > &self, > manifest: &BackupManifest, > name: &str, > ) -> Result { > - let mut tmpfile = std::fs::OpenOptions::new() > - .write(true) > - .read(true) > - .custom_flags(libc::O_TMPFILE) > - .open("/tmp")?; > + let mut tmpfile = crate::tools::create_tmp_file()?; > > self.download(name, &mut tmpfile).await?; > > diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs > index 8a03d8ea..f184c750 100644 > --- a/pbs-client/src/backup_writer.rs > +++ b/pbs-client/src/backup_writer.rs > @@ -1,6 +1,5 @@ > use std::collections::HashSet; > use std::future::Future; > -use std::os::unix::fs::OpenOptionsExt; > use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering}; > use std::sync::{Arc, Mutex}; > > @@ -523,11 +522,7 @@ impl BackupWriter { > manifest: &BackupManifest, > known_chunks: Arc>>, > ) -> Result { > - let mut tmpfile = std::fs::OpenOptions::new() > - .write(true) > - .read(true) > - .custom_flags(libc::O_TMPFILE) > - .open("/tmp")?; > + let mut tmpfile = crate::tools::create_tmp_file()?; > > let param = json!({ "archive-name": archive_name }); > self.h2 > @@ -562,11 +557,7 @@ impl BackupWriter { > manifest: &BackupManifest, > known_chunks: Arc>>, > ) -> Result { > - let mut tmpfile = std::fs::OpenOptions::new() > - .write(true) > - .read(true) > - .custom_flags(libc::O_TMPFILE) > - .open("/tmp")?; > + let mut tmpfile = crate::tools::create_tmp_file()?; > > let param = json!({ "archive-name": archive_name }); > self.h2 > diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs > index 1b0123a3..9a7327ed 100644 > --- a/pbs-client/src/tools/mod.rs > +++ b/pbs-client/src/tools/mod.rs > @@ -3,8 +3,10 @@ use std::collections::HashMap; > use std::env::VarError::{NotPresent, NotUnicode}; > use std::fs::File; > use std::io::{BufRead, BufReader}; > +use std::os::unix::fs::OpenOptionsExt; > use std::os::unix::io::FromRawFd; > use std::process::Command; > +use std::sync::OnceLock; > > use anyhow::{bail, format_err, Context, Error}; > use serde_json::{json, Value}; > @@ -526,3 +528,46 @@ pub fn place_xdg_file( > .and_then(|base| base.place_config_file(file_name).map_err(Error::from)) > .with_context(|| format!("failed to place {} in xdg home", description)) > } > + > +/// Creates a temporary file (created with O_TMPFILE) in either > +/// XDG_CACHE_HOME/proxmox-backup if the directories can be created, or in /tmp. > +pub fn create_tmp_file() -> std::io::Result { > + static TMP_PATH: OnceLock = OnceLock::new(); > + > + let tmp_path = TMP_PATH.get_or_init(|| { > + let base = match base_directories() { > + Ok(v) => v, > + Err(_) => return std::path::PathBuf::from("/tmp"), > + }; > + let cache_home = base.get_cache_home(); > + > + match std::fs::create_dir_all(&cache_home) { > + Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => cache_home, > + Ok(()) => cache_home, > + Err(_) => std::path::PathBuf::from("/tmp"), > + } > + }); In my opinion this closure is quite complex to understand and therefore a little error prone. Could you extract it to one or more additional functions to simplify it? > + > + std::fs::OpenOptions::new() > + .write(true) > + .read(true) > + .custom_flags(libc::O_TMPFILE) > + .open(tmp_path) > +} > + > +#[cfg(test)] > +mod test { > + use std::io::{Read, Seek, Write}; > + > + #[test] > + fn test_create_tmp_file() -> anyhow::Result<()> { > + let mut file = crate::tools::create_tmp_file()?; > + const BYTES: &[u8] = b"Hello, World"; > + file.write(BYTES)?; > + let mut buf = [0; BYTES.len()]; > + file.rewind()?; > + file.read(&mut buf)?; > + assert_eq!(&buf, BYTES); > + Ok(()) > + } > +} According to clippy write does not guarantee that the entire buffer is written and you should use write_all instead. I am not 100% sure if this is an issue here. https://rust-lang.github.io/rust-clippy/master/index.html#/unused_io_amount > diff --git a/proxmox-backup-client/src/catalog.rs b/proxmox-backup-client/src/catalog.rs > index 72b22e67..f3bc4ede 100644 > --- a/proxmox-backup-client/src/catalog.rs > +++ b/proxmox-backup-client/src/catalog.rs > @@ -1,5 +1,4 @@ > use std::io::{Seek, SeekFrom}; > -use std::os::unix::fs::OpenOptionsExt; > use std::sync::Arc; > > use anyhow::{bail, format_err, Error}; > @@ -104,11 +103,7 @@ async fn dump_catalog(param: Value) -> Result { > > let mut reader = BufferedDynamicReader::new(index, chunk_reader); > > - let mut catalogfile = std::fs::OpenOptions::new() > - .write(true) > - .read(true) > - .custom_flags(libc::O_TMPFILE) > - .open("/tmp")?; > + let mut catalogfile = pbs_client::tools::create_tmp_file()?; > > std::io::copy(&mut reader, &mut catalogfile) > .map_err(|err| format_err!("unable to download catalog - {}", err))?; > @@ -196,11 +191,7 @@ async fn catalog_shell(param: Value) -> Result<(), Error> { > ) > .await?; > > - let mut tmpfile = std::fs::OpenOptions::new() > - .write(true) > - .read(true) > - .custom_flags(libc::O_TMPFILE) > - .open("/tmp")?; > + let mut tmpfile = pbs_client::tools::create_tmp_file()?; > > let (manifest, _) = client.download_manifest().await?; > manifest.check_fingerprint(crypt_config.as_ref().map(Arc::as_ref))?; > @@ -240,11 +231,7 @@ async fn catalog_shell(param: Value) -> Result<(), Error> { > most_used, > ); > let mut reader = BufferedDynamicReader::new(index, chunk_reader); > - let mut catalogfile = std::fs::OpenOptions::new() > - .write(true) > - .read(true) > - .custom_flags(libc::O_TMPFILE) > - .open("/tmp")?; > + let mut catalogfile = pbs_client::tools::create_tmp_file()?; > > std::io::copy(&mut reader, &mut catalogfile) > .map_err(|err| format_err!("unable to download catalog - {}", err))?;