Re: [PATCH proxmox-backup v3 07/30] pbs-config: implement encryption key config handling

Daniel Kral
2026-04-15T08:04:26Z

On Wed Apr 15, 2026 at 8:48 AM CEST, Christian Ebner wrote:
> On 4/14/26 4:31 PM, Michael Köppl wrote:
>> 2 comments inline
>>
>> On Tue Apr 14, 2026 at 2:59 PM CEST, Christian Ebner wrote:
>>
>> [snip]
>>
>>> +/// Store the encryption key to file.
>>> +///
>>> +/// Inserts the key in the config and stores it to the given file.
>>> +pub fn store_key(id: &str, key: &KeyConfig) -> Result<(), Error> {
>>> + let _lock = lock_config()?;
>>> + let (mut config, _digest) = config()?;
>>> +
>>> + if config.sections.contains_key(id) {
>>> + bail!("key with id '{id}' already exists.");
>>> + }
>>> +
>>> + let backup_user = crate::backup_user()?;
>>> + let dir_options = CreateOptions::new()
>>> + .perm(Mode::from_bits_truncate(0o0750))
>>> + .owner(Uid::from_raw(0))
>>> + .group(backup_user.gid);
>>> +
>>> + proxmox_sys::fs::ensure_dir_exists(ENCRYPTION_KEYS_DIR, &dir_options, true)?;
>>> +
>>> + let key_path = format!("{ENCRYPTION_KEYS_DIR}{id}.enc");
>>> + let key_lock_path = format!("{key_path}.lck");
>>> +
>>> + // lock to avoid race with key deletion
>>> + open_backup_lockfile(&key_lock_path, None, true)?;
>>
>> This needs to be assigned to a variable, no? Otherwise, the lock would
>> be immediately dropped.
>
> Oh, good catch! Indeed without this the lock would be immediately
> dropped, will be fixed. Thanks!
>

Would it make sense to add a #[must_use = "..."] attribute [0] to
open_backup_lockfile() or even the BackupLockGuard in general or would
it be too strict here?

[0] https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute

>>
>> In other places we have something like let _lock = lock_config()?;
>>