[pbs-devel] [PATCH backup v3] fix-3699: pbs-client/tools: use xdg basedirectories for tmp files

[pbs-devel] [PATCH backup v3] fix-3699: pbs-client/tools: use xdg basedirectories for tmp files

[Maximiliano Sandoval]

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 <m.sandoval@proxmox.com>
---
Differences from v2:
 - Files are created at XDG_CACHE_HOME directly
 - If opening fails, we try opening at /tmp
 - We check if the directory exists as part of the error handling of open()

 pbs-client/src/backup_reader.rs      | 31 ++++++++--------------------
 pbs-client/src/backup_writer.rs      | 13 ++----------
 pbs-client/src/tools/mod.rs          | 26 +++++++++++++++++++++++
 proxmox-backup-client/src/catalog.rs | 19 +++--------------
 4 files changed, 40 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<DataBlobReader<'_, File>, 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<DynamicIndexReader, 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?;
 
@@ -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<FixedIndexReader, 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?;
 
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<Mutex<HashSet<[u8; 32]>>>,
     ) -> Result<FixedIndexReader, 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()?;
 
         let param = json!({ "archive-name": archive_name });
         self.h2
@@ -562,11 +557,7 @@ impl BackupWriter {
         manifest: &BackupManifest,
         known_chunks: Arc<Mutex<HashSet<[u8; 32]>>>,
     ) -> Result<DynamicIndexReader, 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()?;
 
         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..a721fbc4 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,27 @@ 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
+/// if the directory exists, otherwise it will be created in /tmp otherwise.
+pub fn create_tmp_file() -> std::io::Result<std::fs::File> {
+    static TMP_PATH: OnceLock<std::path::PathBuf> = OnceLock::new();
+    let tmp_path = TMP_PATH.get_or_init(|| {
+        xdg::BaseDirectories::new()
+            .map(|base| base.xdg_cache_dir())
+            .unwrap_or_else(|| std::path::PathBuf::from("/tmp"))
+    });
+
+    let mut open_opts_binding = std::fs::OpenOptions::new();
+    let builder = open_opts_binding
+        .write(true)
+        .read(true)
+        .custom_flags(libc::O_TMPFILE);
+    builder.open(tmp_path).or_else(|err| {
+        if tmp_path != std::path::Path::new("/tmp") {
+            builder.open("/tmp")
+        } else {
+            Err(err)
+        }
+    })
+}
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<Value, Error> {
 
     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))?;
-- 
2.39.2

Re: [pbs-devel] [PATCH backup v3] fix-3699: pbs-client/tools: use xdg basedirectories for tmp files

[Thomas Lamprecht]

please keep to the commonly used `fix #id: ...` format for the subject,
the correct that would "client" here, and could be slightly reworded to
better convey what's done here, e.g. something like:

fix #3699: client: prefer xdg cache directory for tmp files

Am 01/02/2024 um 14:49 schrieb Maximiliano Sandoval:
> 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 <m.sandoval@proxmox.com>
> ---
> Differences from v2:
>  - Files are created at XDG_CACHE_HOME directly
>  - If opening fails, we try opening at /tmp
>  - We check if the directory exists as part of the error handling of open()
> 
>  pbs-client/src/backup_reader.rs      | 31 ++++++++--------------------
>  pbs-client/src/backup_writer.rs      | 13 ++----------
>  pbs-client/src/tools/mod.rs          | 26 +++++++++++++++++++++++
>  proxmox-backup-client/src/catalog.rs | 19 +++--------------
>  4 files changed, 40 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

> @@ -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

I'd keep the location info, or better, refer to the underlying
"create_tmp_file" function with a docs link so that it can be easily
checked out w.r.t. semantics.

> +    /// using the provided manifest.



> diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
> index 1b0123a3..a721fbc4 100644
> --- a/pbs-client/src/tools/mod.rs
> +++ b/pbs-client/src/tools/mod.rs

> @@ -526,3 +528,27 @@ 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
> +/// if the directory exists, otherwise it will be created in /tmp otherwise.

duplicate "otherwise", could be fixed up on applying though.

> +pub fn create_tmp_file() -> std::io::Result<std::fs::File> {

Hmm, wondering if "create_xdg_tmp_file" could be slightly better here,
but the fallback behavior is not the best fit with that name, so no
hard feelings from my side.

> +    static TMP_PATH: OnceLock<std::path::PathBuf> = OnceLock::new();
> +    let tmp_path = TMP_PATH.get_or_init(|| {
> +        xdg::BaseDirectories::new()
> +            .map(|base| base.xdg_cache_dir())

I'm really not to sure if the cache one is the best fit, but it certainly
isn't wrong either.
What I'd like though is some sentence w.r.t. choosing this  over the
runtime one in the commit message, i.e., one that states that there isn't
an explicit one for temporary files and that both, run and cache ones,
would be OK choices, but we choose the cache one <arbitrarily|$reasons>

> +            .unwrap_or_else(|| std::path::PathBuf::from("/tmp"))
> +    });
> +
> +    let mut open_opts_binding = std::fs::OpenOptions::new();
> +    let builder = open_opts_binding
> +        .write(true)
> +        .read(true)
> +        .custom_flags(libc::O_TMPFILE);
> +    builder.open(tmp_path).or_else(|err| {
> +        if tmp_path != std::path::Path::new("/tmp") {
> +            builder.open("/tmp")
> +        } else {
> +            Err(err)
> +        }
> +    })
> +}

Re: [pbs-devel] [PATCH backup v3] fix-3699: pbs-client/tools: use xdg basedirectories for tmp files

[Maximiliano Sandoval]

Thomas Lamprecht <t.lamprecht@proxmox.com> writes:

>> +pub fn create_tmp_file() -> std::io::Result<std::fs::File> {
>
> Hmm, wondering if "create_xdg_tmp_file" could be slightly better here,
> but the fallback behavior is not the best fit with that name, so no
> hard feelings from my side.

Since this indeed can create files outside of XDG_CACHE_HOME I opted for
not putting `xdg` in the fn name. It is also a bit more future-proof in
case the function is refactored/modified. No strong opinions anyways.

I will send a v4 today.

--
Maximiliano