Re: [pbs-devel] [PATCH proxmox] sys: file: use renameat2() from `nix` crate

[Christian Ebner <c.ebner@proxmox.com>]

one small nit inline:
> On 11.07.2024 19:07 CEST Christoph Heiss <c.heiss@proxmox.com> wrote:
> 
>  
> Saves us from converting the paths to raw C strings ourselves, thus
> simplifying it quite a bit.
> 
> No functional changes.
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  proxmox-sys/src/fs/file.rs | 51 ++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 29 deletions(-)
> 
> diff --git a/proxmox-sys/src/fs/file.rs b/proxmox-sys/src/fs/file.rs
> index ac513891..3b20c4ea 100644
> --- a/proxmox-sys/src/fs/file.rs
> +++ b/proxmox-sys/src/fs/file.rs
> @@ -10,7 +10,6 @@ use nix::errno::Errno;
>  use nix::fcntl::OFlag;
>  use nix::sys::stat;
>  use nix::unistd;
> -use nix::NixPath;
>  use serde_json::Value;
>  
>  use proxmox_lang::try_block;
> @@ -266,30 +265,32 @@ pub fn atomic_open_or_create_file<P: AsRef<Path>>(
>  
>      // rotate the file into place, but use `RENAME_NOREPLACE`, so in case 2 processes race against
>      // the initialization, the first one wins!
> -    let rename_result = temp_file_name.with_nix_path(|c_file_name| {
> -        path.with_nix_path(|new_path| unsafe {
> -            // This also works on file systems which don't support hardlinks (eg. vfat)
> -            match Errno::result(libc::renameat2(
> -                libc::AT_FDCWD,
> -                c_file_name.as_ptr(),
> -                libc::AT_FDCWD,
> -                new_path.as_ptr(),
> -                libc::RENAME_NOREPLACE,
> -            )) {
> -                Err(Errno::EINVAL) => (), // dumb file system, try `link`+`unlink`
> -                other => return other,
> -            };
> -            // but some file systems don't support `RENAME_NOREPLACE`
> +    let rename_result = match nix::fcntl::renameat2(
> +        None, // AT_FDCWD
> +        &temp_file_name,
> +        None, // AT_FDCWD
> +        path,
> +        nix::fcntl::RenameFlags::RENAME_NOREPLACE,
> +    ) {
> +        Err(Errno::EINVAL) => {
> +            // some file systems don't support `RENAME_NOREPLACE`
>              // so we just use `link` + `unlink` instead
> -            let result = Errno::result(libc::link(c_file_name.as_ptr(), new_path.as_ptr()));
> -            let _ = libc::unlink(c_file_name.as_ptr());
> +            let result = nix::unistd::linkat(
> +                None,
> +                &temp_file_name,
> +                None,
> +                &path.to_path_buf(),
> +                nix::unistd::LinkatFlags::NoSymlinkFollow,

According to https://docs.rs/nix/0.29.0/nix/unistd/type.LinkatFlags.html
`LinkatFlags` will be deprecated with version 0.28. We currently use nix 0.16.1, which already provides the suggested replacment using the `AtFlags` https://docs.rs/nix/0.26.1/nix/fcntl/struct.AtFlags.html.

So it would make sense to already use these instead.

> +            );
> +            let _ = nix::unistd::unlink(&temp_file_name);
>              result
> -        })
> -    });
> +        }
> +        other => other,
> +    };
>  
>      match rename_result {
> -        Ok(Ok(Ok(_))) => Ok(file),
> -        Ok(Ok(Err(err))) => {
> +        Ok(_) => Ok(file),
> +        Err(err) => {
>              // if another process has already raced ahead and created
>              // the file, let's just open theirs instead:
>              let _ = nix::unistd::unlink(&temp_file_name);
> @@ -308,14 +309,6 @@ pub fn atomic_open_or_create_file<P: AsRef<Path>>(
>                  );
>              }
>          }
> -        Ok(Err(err)) => {
> -            let _ = nix::unistd::unlink(&temp_file_name);
> -            bail!("with_nix_path {:?} failed - {}", path, err);
> -        }
> -        Err(err) => {
> -            let _ = nix::unistd::unlink(&temp_file_name);
> -            bail!("with_nix_path {:?} failed - {}", temp_file_name, err);
> -        }
>      }
>  }
>  
> -- 
> 2.45.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel