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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 7FB7E9217E for ; Thu, 1 Feb 2024 12:47:37 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 66622E54F for ; Thu, 1 Feb 2024 12:47:37 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 1 Feb 2024 12:47:36 +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 1C9FD41EA7 for ; Thu, 1 Feb 2024 12:47:36 +0100 (CET) Date: Thu, 01 Feb 2024 12:47:29 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20240131105315.237684-1-m.sandoval@proxmox.com> In-Reply-To: <20240131105315.237684-1-m.sandoval@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1706787171.yz7pdohg3d.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.065 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 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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. [catalog.rs, mod.rs] 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 11:47:37 -0000 On January 31, 2024 11:53 am, 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. >=20 > Signed-off-by: Maximiliano Sandoval > --- > Differences from v1: > - temporal file -> temporary file > - add a test >=20 > 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(-) >=20 > 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; > =20 > 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", descr= iption)) > } > + > +/// 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 =3D OnceLock::new(); > + > + let tmp_path =3D TMP_PATH.get_or_init(|| { > + let base =3D match base_directories() { these base_directories use the prefix 'proxmox-backup', which is not really needed here since we don't want to actually store the file.. > + Ok(v) =3D> v, > + Err(_) =3D> return std::path::PathBuf::from("/tmp"), > + }; > + let cache_home =3D base.get_cache_home(); I do wonder whether XDG_RUNTIME_DIR would be a better fit, but tbh, our use case doesn't really fit either ;) runtime would have the advantage that the specs basically says it must support O_TMPFILE (not explicitly, but "The directory MUST by fully-featured by the standards of the operating system."). the main downside is it might be backed by memory (but so is the current path "/tmp" on most systems) and on some systems it might be misconfigured w.r.t. session handling (although such systems will have plenty of other problems already). > + > + match std::fs::create_dir_all(&cache_home) { without the prefix, we could just test for existence here, instead of (potentially) creating a dir structure where we don't even persist anything= ? > + Err(err) if err.kind() =3D=3D std::io::ErrorKind::AlreadyExi= sts =3D> cache_home, > + Ok(()) =3D> cache_home, > + Err(_) =3D> std::path::PathBuf::from("/tmp"), > + } > + }); > + > + std::fs::OpenOptions::new() > + .write(true) > + .read(true) > + .custom_flags(libc::O_TMPFILE) > + .open(tmp_path) since you don't have a fallback to tmp here, this actually has a chance of breaking systems with XDG_CACHE_HOME or HOME set, but with the full resolved path not supporting O_TMPFILE.. > +} > + > +#[cfg(test)] > +mod test { > + use std::io::{Read, Seek, Write}; > + > + #[test] > + fn test_create_tmp_file() -> anyhow::Result<()> { > + let mut file =3D crate::tools::create_tmp_file()?; > + const BYTES: &[u8] =3D b"Hello, World"; > + file.write(BYTES)?; > + let mut buf =3D [0; BYTES.len()]; > + file.rewind()?; > + file.read(&mut buf)?; > + assert_eq!(&buf, BYTES); > + Ok(()) > + } > +} this test doesn't really test much other than that the environment in which it runs has a configured XDG dir or /tmp with write access and O_TMPFILE support ;) it has the side-effect of potentially creating directories (outside of the build dir) if they don't exist already, which might be undesired.