Re: [pbs-devel] [PATCH proxmox 1/2] new helper atomic_open_or_create_file()

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

one small nit, otherwise this looks okay (and DOES set the permissions).

On July 16, 2021 10:28 am, Dietmar Maurer wrote:
> ---
>  proxmox/src/tools/fs.rs | 86 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/proxmox/src/tools/fs.rs b/proxmox/src/tools/fs.rs
> index 12e96bd..2a93b30 100644
> --- a/proxmox/src/tools/fs.rs
> +++ b/proxmox/src/tools/fs.rs
> @@ -12,9 +12,10 @@ use nix::errno::Errno;
>  use nix::fcntl::OFlag;
>  use nix::sys::stat;
>  use nix::unistd::{self, Gid, Uid};
> +use nix::NixPath;
>  use serde_json::Value;
>  
> -use crate::sys::error::SysResult;
> +use crate::sys::error::{SysError, SysResult};
>  use crate::sys::timer;
>  use crate::tools::fd::Fd;
>  use crate::try_block;
> @@ -187,6 +188,89 @@ pub fn replace_file<P: AsRef<Path>>(
>      Ok(())
>  }
>  
> +/// Like open(2), but allows setting initial data, perm, owner and group
> +///
> +/// Since we need to initialize the file, we also need a solid slow
> +/// path where we create the file. In order to avoid races, we create
> +/// it in a temporary location and rotate it in place.
> +pub fn atomic_open_or_create_file<P: AsRef<Path>>(
> +    path: P,
> +    mut oflag: OFlag,
> +    initial_data: &[u8],
> +    options: CreateOptions,
> +) -> Result<File, Error> {
> +    let path = path.as_ref();
> +
> +    if oflag.contains(OFlag::O_TMPFILE) {
> +        bail!("open {:?} failed - unsupported OFlag O_TMPFILE", path);
> +    }
> +
> +    oflag.remove(OFlag::O_CREAT); // we want to handle CREAT ourselfes
> +
> +    // Note: 'mode' is ignored, because oflag does not contain O_CREAT or O_TMPFILE
> +    match nix::fcntl::open(path, oflag, stat::Mode::empty()) {
> +        Ok(fd) => return Ok(unsafe { File::from_raw_fd(fd) }),
> +        Err(err) => {
> +           if err.not_found() {
> +               // fall thrue -  try to create the file
> +           } else {
> +               bail!("open {:?} failed - {}", path, err);
> +           }
> +        }
> +    }
> +
> +    let (mut file, temp_file_name) = make_tmp_file(path, options)?;

so after this point we have a temp file that requires cleanup

> +
> +    if !initial_data.is_empty() {
> +        file.write_all(initial_data).map_err(|err| {
> +            let _ = nix::unistd::unlink(&temp_file_name);
> +            format_err!(
> +                "writing initial data to {:?} failed - {}",
> +                temp_file_name,
> +                err,
> +            )
> +        })?;
> +    }
> +
> +    // 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 {
> +            let rc = libc::renameat2(
> +                libc::AT_FDCWD,
> +                c_file_name.as_ptr(),
> +                libc::AT_FDCWD,
> +                new_path.as_ptr(),
> +                libc::RENAME_NOREPLACE,
> +            );
> +            nix::errno::Errno::result(rc)
> +        })?
> +    })?;

but here we bubble up the outer Result if it's an error, without any 
cleanup.

> +
> +    match rename_result {
> +        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);
> +
> +            if err.already_exists() {
> +                match nix::fcntl::open(path, oflag, stat::Mode::empty()) {
> +                    Ok(fd) => Ok(unsafe { File::from_raw_fd(fd) }),
> +                    Err(err) => bail!("open {:?} failed - {}", path, err),
> +                }
> +            } else {
> +                bail!(
> +                    "failed to move file at {:?} into place at {:?} - {}",
> +                    temp_file_name,
> +                    path,
> +                    err
> +                );
> +            }
> +        }
> +    }
> +}
> +
>  /// Change ownership of an open file handle
>  pub fn fchown(fd: RawFd, owner: Option<Uid>, group: Option<Gid>) -> Result<(), Error> {
>      // According to the POSIX specification, -1 is used to indicate that owner and group
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
>