From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox 1/2] new helper atomic_open_or_create_file()
Date: Mon, 19 Jul 2021 12:44:27 +0200 [thread overview]
Message-ID: <1626690094.9fpnyum3i1.astroid@nora.none> (raw)
In-Reply-To: <<20210716082834.2354163-1-dietmar@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
>
>
>
prev parent reply other threads:[~2021-07-19 10:44 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-16 8:28 Dietmar Maurer
2021-07-16 8:28 ` [pbs-devel] [PATCH proxmox-backup 1/2] use new atomic_open_or_create_file Dietmar Maurer
[not found] ` <<20210716082834.2354163-2-dietmar@proxmox.com>
2021-07-19 10:45 ` Fabian Grünbichler
2021-07-16 8:28 ` [pbs-devel] [PATCH proxmox-backup 2/2] add helpers to write configuration files Dietmar Maurer
2021-07-16 8:28 ` [pbs-devel] [PATCH proxmox 2/2] open_file_locked: add options parameter (CreateOptions) Dietmar Maurer
[not found] ` <<20210716082834.2354163-4-dietmar@proxmox.com>
2021-07-19 10:44 ` Fabian Grünbichler
[not found] ` <<20210716082834.2354163-1-dietmar@proxmox.com>
2021-07-19 10:44 ` Fabian Grünbichler [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1626690094.9fpnyum3i1.astroid@nora.none \
--to=f.gruenbichler@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox