Re: [pbs-devel] [PATCH proxmox v2 2/2] sys: open directories with O_CLOEXEC

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

one small nit inline, otherwise:

Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

On November 29, 2024 3:28 pm, Dominik Csapak wrote:
> so they don't linger around in case of a daemon reload.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> new in v2
>  proxmox-sys/src/fd.rs     |  2 +-
>  proxmox-sys/src/fs/dir.rs | 15 +++++++++------
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/proxmox-sys/src/fd.rs b/proxmox-sys/src/fd.rs
> index 8d85bd2e..386e4222 100644
> --- a/proxmox-sys/src/fd.rs
> +++ b/proxmox-sys/src/fd.rs
> @@ -24,7 +24,7 @@ pub fn change_cloexec(fd: RawFd, on: bool) -> Result<(), anyhow::Error> {
>  }
>  
>  pub(crate) fn cwd() -> Result<OwnedFd, nix::Error> {
> -    open(".", OFlag::O_DIRECTORY, stat::Mode::empty())
> +    open(".", crate::fs::DIR_FLAGS, stat::Mode::empty())
>  }
>  
>  pub fn open<P>(path: &P, oflag: OFlag, mode: Mode) -> Result<OwnedFd, nix::Error>
> diff --git a/proxmox-sys/src/fs/dir.rs b/proxmox-sys/src/fs/dir.rs
> index c903ab87..a093ed99 100644
> --- a/proxmox-sys/src/fs/dir.rs
> +++ b/proxmox-sys/src/fs/dir.rs
> @@ -14,6 +14,9 @@ use proxmox_lang::try_block;
>  
>  use crate::fs::{fchown, CreateOptions};
>  
> +/// The default [`OFlag`] we want to use when opening directories.
> +pub(crate) const DIR_FLAGS: OFlag = OFlag::O_DIRECTORY.union(OFlag::O_CLOEXEC);

nit: I think I'd prefer a plain `|` here (they are the same in the
bitflags crate, which this is under the hood).

> +
>  /// Creates directory at the provided path with specified ownership.
>  ///
>  /// Errors if the directory already exists.
> @@ -66,7 +69,7 @@ pub fn ensure_dir_exists<P: AsRef<Path>>(
>          Err(err) => bail!("unable to create directory {path:?} - {err}",),
>      }
>  
> -    let fd = nix::fcntl::open(path, OFlag::O_DIRECTORY, stat::Mode::empty())
> +    let fd = nix::fcntl::open(path, DIR_FLAGS, stat::Mode::empty())
>          .map(|fd| unsafe { OwnedFd::from_raw_fd(fd) })
>          .map_err(|err| format_err!("unable to open created directory {path:?} - {err}"))?;
>      // umask defaults to 022 so make sure the mode is fully honowed:
> @@ -120,7 +123,7 @@ fn create_path_do(
>          Some(Component::Prefix(_)) => bail!("illegal prefix path component encountered"),
>          Some(Component::RootDir) => {
>              let _ = iter.next();
> -            crate::fd::open(c"/", OFlag::O_DIRECTORY, stat::Mode::empty())?
> +            crate::fd::open(c"/", DIR_FLAGS, stat::Mode::empty())?
>          }
>          Some(Component::CurDir) => {
>              let _ = iter.next();
> @@ -128,7 +131,7 @@ fn create_path_do(
>          }
>          Some(Component::ParentDir) => {
>              let _ = iter.next();
> -            crate::fd::open(c"..", OFlag::O_DIRECTORY, stat::Mode::empty())?
> +            crate::fd::open(c"..", DIR_FLAGS, stat::Mode::empty())?
>          }
>          Some(Component::Normal(_)) => {
>              // simply do not advance the iterator, heavy lifting happens in create_path_at_do()
> @@ -154,7 +157,7 @@ fn create_path_at_do(
>              None => return Ok(created),
>  
>              Some(Component::ParentDir) => {
> -                at = crate::fd::openat(&at, c"..", OFlag::O_DIRECTORY, stat::Mode::empty())?;
> +                at = crate::fd::openat(&at, c"..", DIR_FLAGS, stat::Mode::empty())?;
>              }
>  
>              Some(Component::Normal(path)) => {
> @@ -175,7 +178,7 @@ fn create_path_at_do(
>                      Err(e) => return Err(e.into()),
>                      Ok(_) => true,
>                  };
> -                at = crate::fd::openat(&at, path, OFlag::O_DIRECTORY, stat::Mode::empty())?;
> +                at = crate::fd::openat(&at, path, DIR_FLAGS, stat::Mode::empty())?;
>  
>                  if let (true, Some(opts)) = (created, opts) {
>                      if opts.owner.is_some() || opts.group.is_some() {
> @@ -222,7 +225,7 @@ pub fn make_tmp_dir<P: AsRef<Path>>(
>  
>      if let Some(options) = options {
>          if let Err(err) = try_block!({
> -            let mut fd = crate::fd::open(&path, OFlag::O_DIRECTORY, stat::Mode::empty())?;
> +            let mut fd = crate::fd::open(&path, DIR_FLAGS, stat::Mode::empty())?;
>              options.apply_to(&mut fd, &path)?;
>              Ok::<(), Error>(())
>          }) {
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> 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