[PATCH v1 proxmox 0/2] improve proxmox-sys::fs::replace_file

[PATCH v1 proxmox 0/2] improve proxmox-sys::fs::replace_file

[Robert Obkircher]

A minor cleanup and support for WORM file systems.

Robert Obkircher (2):
  proxmox-sys: avoid unnecessary conversion in replace_file
  fix #7690: proxmox-sys: close fd before rename/unlink in replace_file

 proxmox-sys/src/fs/file.rs | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

-- 
2.47.3

[PATCH v1 proxmox 1/2] proxmox-sys: avoid unnecessary conversion in replace_file

[Robert Obkircher]

Since make_tmp_file already returns a File there is no need to convert
to a raw file descriptor and back.

Fixes: 26c06df4 ("make_tmp_file: return File instead of Fd")
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 proxmox-sys/src/fs/file.rs | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/proxmox-sys/src/fs/file.rs b/proxmox-sys/src/fs/file.rs
index f0939381..533e0896 100644
--- a/proxmox-sys/src/fs/file.rs
+++ b/proxmox-sys/src/fs/file.rs
@@ -1,6 +1,6 @@
 use std::fs::File;
 use std::io::{self, BufRead, BufReader, Write};
-use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd};
+use std::os::unix::io::{AsRawFd, FromRawFd};
 use std::path::{Path, PathBuf};
 #[cfg(feature = "timer")]
 use std::time::Duration;
@@ -177,9 +177,7 @@ pub fn replace_file<P: AsRef<Path>>(
     options: CreateOptions,
     fsync: bool,
 ) -> Result<(), Error> {
-    let (fd, tmp_path) = make_tmp_file(&path, options)?;
-
-    let mut file = unsafe { File::from_raw_fd(fd.into_raw_fd()) };
+    let (mut file, tmp_path) = make_tmp_file(&path, options)?;
 
     if let Err(err) = file.write_all(data) {
         let _ = unistd::unlink(&tmp_path);
-- 
2.47.3

[PATCH v1 proxmox 2/2] fix #7690: proxmox-sys: close fd before rename/unlink in replace_file

[Robert Obkircher]

Fix the rename operation on WORM file systems and allow FUSE file
systems to unlink without creating temporary .fuse_hidden files.

Closing the fd earlier shouldn't have any noticeable effects on normal
file systems, because the path-based rename and unlink operations are
completely separate from the content.

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7690
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 proxmox-sys/src/fs/file.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/proxmox-sys/src/fs/file.rs b/proxmox-sys/src/fs/file.rs
index 533e0896..03223e5a 100644
--- a/proxmox-sys/src/fs/file.rs
+++ b/proxmox-sys/src/fs/file.rs
@@ -180,6 +180,7 @@ pub fn replace_file<P: AsRef<Path>>(
     let (mut file, tmp_path) = make_tmp_file(&path, options)?;
 
     if let Err(err) = file.write_all(data) {
+        drop(file);
         let _ = unistd::unlink(&tmp_path);
         bail!("write failed: {}", err);
     }
@@ -187,11 +188,16 @@ pub fn replace_file<P: AsRef<Path>>(
     if fsync {
         // make sure data is on disk
         if let Err(err) = nix::unistd::fsync(file.as_raw_fd()) {
+            drop(file);
             let _ = unistd::unlink(&tmp_path);
             bail!("fsync failed: {}", err);
         }
     }
 
+    // Allow WORM file systems to commit the contents before the rename
+    // and prevent temporary .fuse_hidden* files created by unlink.
+    drop(file);
+
     if let Err(err) = std::fs::rename(&tmp_path, &path) {
         let _ = unistd::unlink(&tmp_path);
         bail!(
-- 
2.47.3

Re: [PATCH v1 proxmox 1/2] proxmox-sys: avoid unnecessary conversion in replace_file

[Christian Ebner]

On 6/17/26 12:49 PM, Robert Obkircher wrote:
> Since make_tmp_file already returns a File there is no need to convert
> to a raw file descriptor and back.
> 
> Fixes: 26c06df4 ("make_tmp_file: return File instead of Fd")
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

Re: [PATCH v1 proxmox 2/2] fix #7690: proxmox-sys: close fd before rename/unlink in replace_file

[Christian Ebner]

On 6/17/26 12:49 PM, Robert Obkircher wrote:
> Fix the rename operation on WORM file systems and allow FUSE file
> systems to unlink without creating temporary .fuse_hidden files.

question: what about other helpers using a similar pattern when removing 
a temp file in error case, e.g. unlink() after file content writing or 
fsync() failed while still holding an open file descriptor in 
atomic_open_or_create_file()? These will produce the .fuse_hidden files 
as well?

> Closing the fd earlier shouldn't have any noticeable effects on normal
> file systems, because the path-based rename and unlink operations are
> completely separate from the content.
> 
> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7690
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>   proxmox-sys/src/fs/file.rs | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/proxmox-sys/src/fs/file.rs b/proxmox-sys/src/fs/file.rs
> index 533e0896..03223e5a 100644
> --- a/proxmox-sys/src/fs/file.rs
> +++ b/proxmox-sys/src/fs/file.rs
> @@ -180,6 +180,7 @@ pub fn replace_file<P: AsRef<Path>>(
>       let (mut file, tmp_path) = make_tmp_file(&path, options)?;
>   
>       if let Err(err) = file.write_all(data) {
> +        drop(file);
>           let _ = unistd::unlink(&tmp_path);
>           bail!("write failed: {}", err);
>       }
> @@ -187,11 +188,16 @@ pub fn replace_file<P: AsRef<Path>>(
>       if fsync {
>           // make sure data is on disk
>           if let Err(err) = nix::unistd::fsync(file.as_raw_fd()) {
> +            drop(file);
>               let _ = unistd::unlink(&tmp_path);
>               bail!("fsync failed: {}", err);
>           }
>       }
>   
> +    // Allow WORM file systems to commit the contents before the rename
> +    // and prevent temporary .fuse_hidden* files created by unlink.
> +    drop(file);
> +
>       if let Err(err) = std::fs::rename(&tmp_path, &path) {
>           let _ = unistd::unlink(&tmp_path);
>           bail!(

Re: [PATCH v1 proxmox 2/2] fix #7690: proxmox-sys: close fd before rename/unlink in replace_file

[Robert Obkircher]

On 29.06.26 13:18, Christian Ebner wrote:
> On 6/17/26 12:49 PM, Robert Obkircher wrote:
>> Fix the rename operation on WORM file systems and allow FUSE file
>> systems to unlink without creating temporary .fuse_hidden files.
>
> question: what about other helpers using a similar pattern when
> removing a temp file in error case, e.g. unlink() after file content
> writing or fsync() failed while still holding an open file
> descriptor in atomic_open_or_create_file()? These will produce the
> .fuse_hidden files as well? 
I think so, but .fuse_hidden files are not really harmful, and it
seems like we mostly use that function for lockfiles outside of
datastores.

Should I write a test to verify when .fuse_hidden files are created? I
didn't bother because I wasn't sure how to create a fuse file system
in a test.

But it also seems a bit strange to me that we use mkostemp and
path-based renames in the first place. Creating the file
with O_TMPFILE and linking it into place would definitely be safer.

>
>
> [...]

Re: [PATCH v1 proxmox 2/2] fix #7690: proxmox-sys: close fd before rename/unlink in replace_file

[Christian Ebner]

On 6/30/26 3:24 PM, Robert Obkircher wrote:
> 
> On 29.06.26 13:18, Christian Ebner wrote:
>> On 6/17/26 12:49 PM, Robert Obkircher wrote:
>>> Fix the rename operation on WORM file systems and allow FUSE file
>>> systems to unlink without creating temporary .fuse_hidden files.
>>
>> question: what about other helpers using a similar pattern when
>> removing a temp file in error case, e.g. unlink() after file content
>> writing or fsync() failed while still holding an open file
>> descriptor in atomic_open_or_create_file()? These will produce the
>> .fuse_hidden files as well?
> I think so, but .fuse_hidden files are not really harmful, and it
> seems like we mostly use that function for lockfiles outside of
> datastores.

Okay, thanks for clarification. Was unsure if these could cause issues 
for the mentioned WORM filesystem implementation as well, so wanted to 
mention these as well.

> Should I write a test to verify when .fuse_hidden files are created? I
> didn't bother because I wasn't sure how to create a fuse file system
> in a test.

Not really worth the effort for the time being, unless we do want to 
explicitly support such FUSE based filesystems in the near future. 
Correct me if I'm wrong, but I'm not aware of active efforts to do so.

> But it also seems a bit strange to me that we use mkostemp and
> path-based renames in the first place. Creating the file
> with O_TMPFILE and linking it into place would definitely be safer.