[pbs-devel] [PATCH 1/2] add tempfile helper without O_TMPFILE or memfd_create

[pbs-devel] [PATCH 1/2] add tempfile helper without O_TMPFILE or memfd_create

[Mira Limbeck]

As WSL does not support O_TMPFILE nor memfd_create, we need a different
way to create tempfiles if we want to support windows host backups
through WSL (without ADS).
The workaround is to use mkstemp in /tmp and unlinking the file
right after creation.

Add FIXME comment to change it back to O_TMPFILE once we no longer require
the mkstemp workaround for windows support.

Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
---
 src/tools.rs          |  1 +
 src/tools/tempfile.rs | 13 +++++++++++++
 2 files changed, 14 insertions(+)
 create mode 100644 src/tools/tempfile.rs

diff --git a/src/tools.rs b/src/tools.rs
index 44db796d..5e892d41 100644
--- a/src/tools.rs
+++ b/src/tools.rs
@@ -30,6 +30,7 @@ pub mod fs;
 pub mod format;
 pub mod lru_cache;
 pub mod runtime;
+pub mod tempfile;
 pub mod ticket;
 pub mod timer;
 pub mod statistics;
diff --git a/src/tools/tempfile.rs b/src/tools/tempfile.rs
new file mode 100644
index 00000000..a91ce806
--- /dev/null
+++ b/src/tools/tempfile.rs
@@ -0,0 +1,13 @@
+use nix::unistd::{mkstemp, unlink};
+use std::fs::File;
+use std::os::unix::io::FromRawFd;
+use anyhow;
+
+// FIXME change to O_TMPFILE once a native windows backup client exists
+pub fn tempfile() -> anyhow::Result<File> {
+    let (fd, path) = mkstemp("/tmp/proxmox-backup-client_XXXXXX")?;
+    unlink(path.as_path())?;
+    let file = unsafe { File::from_raw_fd(fd) };
+
+    Ok(file)
+}
-- 
2.20.1

[pbs-devel] [PATCH 2/2] replace O_TMPFILE file creation with tempfile()

[Mira Limbeck]

To make the proxmox-backup-client work under WSL replace all occurences
of open() with O_TMPFILE with the custom tempfile() function.

Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
---
 src/bin/proxmox_backup_client/catalog.rs | 20 ++++----------------
 src/client/backup_reader.rs              | 20 ++++----------------
 src/client/backup_writer.rs              | 14 +++-----------
 3 files changed, 11 insertions(+), 43 deletions(-)

diff --git a/src/bin/proxmox_backup_client/catalog.rs b/src/bin/proxmox_backup_client/catalog.rs
index 1c0865e6..34f00ec4 100644
--- a/src/bin/proxmox_backup_client/catalog.rs
+++ b/src/bin/proxmox_backup_client/catalog.rs
@@ -1,4 +1,3 @@
-use std::os::unix::fs::OpenOptionsExt;
 use std::io::{Seek, SeekFrom};
 use std::sync::Arc;
 
@@ -8,6 +7,7 @@ use serde_json::Value;
 use proxmox::api::{api, cli::*};
 
 use proxmox_backup::tools;
+use proxmox_backup::tools::tempfile::tempfile;
 
 use proxmox_backup::client::*;
 
@@ -101,11 +101,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 = tempfile()?;
 
     std::io::copy(&mut reader, &mut catalogfile)
         .map_err(|err| format_err!("unable to download catalog - {}", err))?;
@@ -190,11 +186,7 @@ async fn catalog_shell(param: Value) -> Result<(), Error> {
         true,
     ).await?;
 
-    let mut tmpfile = std::fs::OpenOptions::new()
-        .write(true)
-        .read(true)
-        .custom_flags(libc::O_TMPFILE)
-        .open("/tmp")?;
+    let mut tmpfile = tempfile()?;
 
     let (manifest, _) = client.download_manifest().await?;
 
@@ -218,11 +210,7 @@ async fn catalog_shell(param: Value) -> Result<(), Error> {
     let most_used = index.find_most_used_chunks(8);
     let chunk_reader = RemoteChunkReader::new(client.clone(), crypt_config, 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 = tempfile()?;
 
     std::io::copy(&mut reader, &mut catalogfile)
         .map_err(|err| format_err!("unable to download catalog - {}", err))?;
diff --git a/src/client/backup_reader.rs b/src/client/backup_reader.rs
index b0b43c38..ae0e5495 100644
--- a/src/client/backup_reader.rs
+++ b/src/client/backup_reader.rs
@@ -2,7 +2,6 @@ use anyhow::{format_err, Error};
 use std::io::{Read, Write, Seek, SeekFrom};
 use std::fs::File;
 use std::sync::Arc;
-use std::os::unix::fs::OpenOptionsExt;
 
 use chrono::{DateTime, Utc};
 use futures::future::AbortHandle;
@@ -11,6 +10,7 @@ use serde_json::{json, Value};
 use proxmox::tools::digest_to_hex;
 
 use crate::backup::*;
+use crate::tools::tempfile::tempfile;
 
 use super::{HttpClient, H2Client};
 
@@ -148,11 +148,7 @@ impl BackupReader {
         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 = tempfile()?;
 
         self.download(name, &mut tmpfile).await?;
 
@@ -174,11 +170,7 @@ impl BackupReader {
         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 = tempfile()?;
 
         self.download(name, &mut tmpfile).await?;
 
@@ -202,11 +194,7 @@ impl BackupReader {
         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 = tempfile()?;
 
         self.download(name, &mut tmpfile).await?;
 
diff --git a/src/client/backup_writer.rs b/src/client/backup_writer.rs
index 7e5adb3c..39912b50 100644
--- a/src/client/backup_writer.rs
+++ b/src/client/backup_writer.rs
@@ -1,5 +1,4 @@
 use std::collections::HashSet;
-use std::os::unix::fs::OpenOptionsExt;
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::sync::{Arc, Mutex};
 
@@ -17,6 +16,7 @@ use proxmox::tools::digest_to_hex;
 use super::merge_known_chunks::{MergedChunkInfo, MergeKnownChunks};
 use crate::backup::*;
 use crate::tools::format::HumanByte;
+use crate::tools::tempfile::tempfile;
 
 use super::{HttpClient, H2Client};
 
@@ -408,11 +408,7 @@ impl BackupWriter {
         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 = tempfile()?;
 
         let param = json!({ "archive-name": archive_name });
         self.h2.download("previous", Some(param), &mut tmpfile).await?;
@@ -443,11 +439,7 @@ impl BackupWriter {
         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 = tempfile()?;
 
         let param = json!({ "archive-name": archive_name });
         self.h2.download("previous", Some(param), &mut tmpfile).await?;
-- 
2.20.1

Re: [pbs-devel] [PATCH 1/2] add tempfile helper without O_TMPFILE or memfd_create

[Fabian Grünbichler]

On July 23, 2020 11:38 am, Mira Limbeck wrote:
> As WSL does not support O_TMPFILE nor memfd_create, we need a different
> way to create tempfiles if we want to support windows host backups
> through WSL (without ADS).
> The workaround is to use mkstemp in /tmp and unlinking the file
> right after creation.
> 
> Add FIXME comment to change it back to O_TMPFILE once we no longer require
> the mkstemp workaround for windows support.
> 
> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
>  src/tools.rs          |  1 +
>  src/tools/tempfile.rs | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
>  create mode 100644 src/tools/tempfile.rs
> 
> diff --git a/src/tools.rs b/src/tools.rs
> index 44db796d..5e892d41 100644
> --- a/src/tools.rs
> +++ b/src/tools.rs
> @@ -30,6 +30,7 @@ pub mod fs;
>  pub mod format;
>  pub mod lru_cache;
>  pub mod runtime;
> +pub mod tempfile;
>  pub mod ticket;
>  pub mod timer;
>  pub mod statistics;
> diff --git a/src/tools/tempfile.rs b/src/tools/tempfile.rs
> new file mode 100644
> index 00000000..a91ce806
> --- /dev/null
> +++ b/src/tools/tempfile.rs
> @@ -0,0 +1,13 @@
> +use nix::unistd::{mkstemp, unlink};
> +use std::fs::File;
> +use std::os::unix::io::FromRawFd;
> +use anyhow;
> +
> +// FIXME change to O_TMPFILE once a native windows backup client exists

shouldn't we try O_TMPFILE first, and only if it fails fall back to 
mkstemp? or guard this with a feature ("wsl")? also not sure whether all 
current (and future) callers are okay with the different semantics (with 
mkstemp the file is briefly available via a path even if you unlink it 
directly afterwards..).

it also might be better off in proxmox instead of proxmox-backup, it's a 
very generic function after all..

> +pub fn tempfile() -> anyhow::Result<File> {
> +    let (fd, path) = mkstemp("/tmp/proxmox-backup-client_XXXXXX")?;
> +    unlink(path.as_path())?;
> +    let file = unsafe { File::from_raw_fd(fd) };
> +
> +    Ok(file)
> +}
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
>