From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id C087B86ED for ; Tue, 22 Aug 2023 10:39:10 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A238D9A4F for ; Tue, 22 Aug 2023 10:39:10 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 22 Aug 2023 10:39:09 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id EDF114326E for ; Tue, 22 Aug 2023 10:39:08 +0200 (CEST) Date: Tue, 22 Aug 2023 10:39:07 +0200 From: Wolfgang Bumiller To: Lukas Wagner Cc: pve-devel@lists.proxmox.com Message-ID: <2rcro4k2bheh4vrr74ryju4ptzeiu4yxefhywzpyn3edlue5vf@267yvx3xhomr> References: <20230821134444.620021-1-l.wagner@proxmox.com> <20230821134444.620021-3-l.wagner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230821134444.620021-3-l.wagner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.053 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment PROLO_LEO1 0.1 Meta Catches all Leo drug variations so far SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [dir.rs] Subject: Re: [pve-devel] [RFC proxmox 2/7] sys: add make_tmp_dir X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Aug 2023 08:39:10 -0000 On Mon, Aug 21, 2023 at 03:44:39PM +0200, Lukas Wagner wrote: > Under the hood, this function calls `mkdtemp` from libc. Unfortunatly > the nix crate did not provide bindings for this function, so we have > to call into libc directly. > > Signed-off-by: Lukas Wagner > --- > proxmox-sys/src/fs/dir.rs | 73 +++++++++++++++++++++++++++++++++++++-- > 1 file changed, 71 insertions(+), 2 deletions(-) > > diff --git a/proxmox-sys/src/fs/dir.rs b/proxmox-sys/src/fs/dir.rs > index 6aee316..72bf1ad 100644 > --- a/proxmox-sys/src/fs/dir.rs > +++ b/proxmox-sys/src/fs/dir.rs > @@ -1,6 +1,8 @@ > -use std::ffi::CStr; > +use std::ffi::{CStr, CString, OsStr}; > +use std::fs::File; > +use std::os::unix::ffi::OsStrExt; > use std::os::unix::io::{AsRawFd, OwnedFd}; > -use std::path::Path; > +use std::path::{Path, PathBuf}; > > use anyhow::{bail, Error}; > use nix::errno::Errno; > @@ -8,6 +10,8 @@ use nix::fcntl::OFlag; > use nix::sys::stat; > use nix::unistd; > > +use proxmox_lang::try_block; > + > use crate::fs::{fchown, CreateOptions}; > > /// Creates directory at the provided path with specified ownership. > @@ -152,6 +156,54 @@ fn create_path_at_do( > } > } > > +/// Create a temporary directory. > +/// > +/// `prefix` determines where the temporary directory will be created. For instance, if > +/// `prefix` is `/tmp`, on success the function will return a path in the style of > +/// `/tmp/tmp_XXXXXX`, where X stands for a random string, ensuring that the path is unique. > +/// > +/// By default, the created directory has `0o700` permissions. If this is not desired, custom > +/// [`CreateOptions`] can be passed via the `option` parameter. > +pub fn make_tmp_dir>( > + prefix: P, > + options: Option, > +) -> Result { > + let mut template = prefix.as_ref().to_owned(); > + template = template.join("tmp_XXXXXX"); > + let template = CString::new(template.into_os_string().as_bytes())?; > + > + let raw_template_buffer = template.into_raw(); ^ This might be shorter without going over the `CString` type with just a `Vec` with an explicit `.push(0)` without temporarily giving up ownership for the `mkdtemp` call. > + > + let path = unsafe { > + let raw_returned_buffer = libc::mkdtemp(raw_template_buffer); > + if raw_returned_buffer.is_null() { Need to add let err = io::Error::last_os_error(); as the very first thing you do in this branch. Never give any external libraries a chance to mess with your `errno` values before you use them, even `std` ;-) > + // The returned pointer points to the same buffer, so in case > + // of an error we need to make sure to claim it back to that > + // it is freed properly. > + drop(CString::from_raw(raw_template_buffer)); ^ but I think we could avoid this - but as long as you fix up the `errno` usage the CString code can also just stay this way, no strong feelings there. > + return Err(std::io::Error::last_os_error().into()); > + } > + CString::from_raw(raw_returned_buffer) > + }; > + > + let path = OsStr::from_bytes(path.as_bytes()); > + let path = PathBuf::from(path); ^ This seems like there should be a cheap non-copying version: PathBuf::from(OsString::from_vec(path.into_bytes())) ? > + > + if let Some(options) = options { > + if let Err(err) = try_block!({ > + let fd = crate::fd::open(&path, OFlag::O_DIRECTORY, stat::Mode::empty())?; > + let mut file = File::from(fd); > + options.apply_to(&mut file, &path)?; ^ Huh, just noticing this weirdness, can we fix up the apply_to API to take a `RawFd` or `&BorrowedFd` instead of a `File`? This is... not a file... :-) And also `CreateOptions` doesn't really need it to be mutable ;-) > + Ok::<(), Error>(()) > + }) { > + let _ = unistd::unlink(&path); ^ This calls `unlink(2)` which does not remove directories. You need to use either `std::fs::remove_dir()` or `unlinkat` with `UnlinkatFlags::RemoveDir`. Also, please also log the error if this fails. > + bail!("could not apply create options to new temporary directory: {err}"); > + } > + } > + > + Ok(path) > +} > + > #[cfg(test)] > mod tests { > use super::*; > @@ -169,4 +221,21 @@ mod tests { > ) > .expect("expected create_path to work"); > } > + > + #[test] > + fn test_make_tmp_dir() -> Result<(), Error> { > + let options = CreateOptions::new() > + .owner(unistd::Uid::effective()) > + .group(unistd::Gid::effective()) > + .perm(stat::Mode::from_bits_truncate(0o755)); > + > + let path = make_tmp_dir("/tmp", Some(options))?; > + > + assert!(path.exists()); > + assert!(path.is_dir()); > + > + std::fs::remove_dir_all(&path)?; > + > + Ok(()) > + } > } > -- > 2.39.2