all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH backup v2] fix-3699: pbs-client/tools: use xdg basedirectories for tmp files
@ 2024-01-31 10:53 Maximiliano Sandoval
  2024-02-01 10:23 ` Philipp Hufnagl
  2024-02-01 11:47 ` Fabian Grünbichler
  0 siblings, 2 replies; 3+ messages in thread
From: Maximiliano Sandoval @ 2024-01-31 10:53 UTC (permalink / raw)
  To: pbs-devel

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 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<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..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<std::fs::File> {
+    static TMP_PATH: OnceLock<std::path::PathBuf> = 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"),
+        }
+    });
+
+    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(())
+    }
+}
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





^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pbs-devel] [PATCH backup v2] fix-3699: pbs-client/tools: use xdg basedirectories for tmp files
  2024-01-31 10:53 [pbs-devel] [PATCH backup v2] fix-3699: pbs-client/tools: use xdg basedirectories for tmp files Maximiliano Sandoval
@ 2024-02-01 10:23 ` Philipp Hufnagl
  2024-02-01 11:47 ` Fabian Grünbichler
  1 sibling, 0 replies; 3+ messages in thread
From: Philipp Hufnagl @ 2024-02-01 10:23 UTC (permalink / raw)
  To: pbs-devel

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 <m.sandoval@proxmox.com>
> ---
> 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<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..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<std::fs::File> {
> +    static TMP_PATH: OnceLock<std::path::PathBuf> = 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<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))?;




^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pbs-devel] [PATCH backup v2] fix-3699: pbs-client/tools: use xdg basedirectories for tmp files
  2024-01-31 10:53 [pbs-devel] [PATCH backup v2] fix-3699: pbs-client/tools: use xdg basedirectories for tmp files Maximiliano Sandoval
  2024-02-01 10:23 ` Philipp Hufnagl
@ 2024-02-01 11:47 ` Fabian Grünbichler
  1 sibling, 0 replies; 3+ messages in thread
From: Fabian Grünbichler @ 2024-02-01 11:47 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion

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.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
> 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/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<std::fs::File> {
> +    static TMP_PATH: OnceLock<std::path::PathBuf> = OnceLock::new();
> +
> +    let tmp_path = TMP_PATH.get_or_init(|| {
> +        let base = 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) => v,
> +            Err(_) => return std::path::PathBuf::from("/tmp"),
> +        };
> +        let cache_home = 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() == std::io::ErrorKind::AlreadyExists => cache_home,
> +            Ok(()) => cache_home,
> +            Err(_) => 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 = 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(())
> +    }
> +}

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.




^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2024-02-01 11:47 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-31 10:53 [pbs-devel] [PATCH backup v2] fix-3699: pbs-client/tools: use xdg basedirectories for tmp files Maximiliano Sandoval
2024-02-01 10:23 ` Philipp Hufnagl
2024-02-01 11:47 ` Fabian Grünbichler

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal