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

[Fabian Grünbichler]

On December 2, 2024 3:55 pm, Dominik Csapak wrote:
> On 12/2/24 15:01, Fabian Grünbichler wrote:
>> one small nit inline, otherwise:
>> 
>> Reviewed-by: Fabian Grünbichler <f.gruenbichler at 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 at 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).
>> 
> 
> had the same thought at first, but the `BitOr` traits (and i guess all traits) are not
> const, so we can't directly to this here.
> 
> what would be possible is this:
> 
> ---
> pub(crate) const DIR_FLAGS: OFlag =
>      OFlag::from_bits_truncate(OFlag::O_DIRECTORY.bits() | OFlag::O_CLOEXEC.bits());
> ---
> 
> which is IMHO even uglier than using `.union()` ...

definitely uglier, and yes, I totally missed that:

https://github.com/rust-lang/rust-project-goals/issues/106
https://github.com/rust-lang/rust/issues/67792

and

https://github.com/bitflags/bitflags/issues/180

the latter which explicitly calls out `.union` and friends being added
to work around this limitation..