[pbs-devel] [RFC PATCH proxmox-backup 1/2] pbs-config: use trait object for the backup lock guard

[pbs-devel] [RFC PATCH proxmox-backup 1/2] pbs-config: use trait object for the backup lock guard

[Dominik Csapak]

instead of a fixed type. The old implementation is now
BackupLockGuardImpl and implements the trait.

At the same time, introduce a type alias with the same name as the
previous struct, so that the users of it do not have to change anything.

This makes it possible for us to have a different lock implementation
for e.g. tests (where we do not actually want to lock)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-config/src/lib.rs | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/pbs-config/src/lib.rs b/pbs-config/src/lib.rs
index 9d8c730d..604fe9d7 100644
--- a/pbs-config/src/lib.rs
+++ b/pbs-config/src/lib.rs
@@ -16,7 +16,14 @@ pub fn backup_group() -> Result<nix::unistd::Group, Error> {
     pbs_tools::sys::query_group(BACKUP_GROUP_NAME)?
         .ok_or_else(|| format_err!("Unable to lookup '{}' group.", BACKUP_GROUP_NAME))
 }
-pub struct BackupLockGuard(std::fs::File);
+
+pub trait BackupLockGuardTrait: Send + Sync + Unpin + std::panic::UnwindSafe + std::panic::RefUnwindSafe { }
+
+struct BackupLockGuardImpl(std::fs::File);
+
+impl BackupLockGuardTrait for BackupLockGuardImpl {}
+
+pub type BackupLockGuard = Box<dyn BackupLockGuardTrait>;
 
 /// Open or create a lock file owned by user "backup" and lock it.
 ///
@@ -39,7 +46,7 @@ pub fn open_backup_lockfile<P: AsRef<std::path::Path>>(
     let timeout = timeout.unwrap_or(std::time::Duration::new(10, 0));
 
     let file = proxmox::tools::fs::open_file_locked(&path, timeout, exclusive, options)?;
-    Ok(BackupLockGuard(file))
+    Ok(Box::new(BackupLockGuardImpl(file)))
 }
 
 /// Atomically write data to file owned by "root:backup" with permission "0640"
-- 
2.30.2

[pbs-devel] [RFC PATCH proxmox-backup 2/2] tape/inventory: fix the tape tests as user by mocking the lock

[Dominik Csapak]

locking during the tests as regular user failed because we try to
chown to the backup user (which is not always possible).

Instead, do not lock at all, by mocking the 'open_backup_lockfile' with
an empty struct that implements the locking trait

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/tape/inventory.rs | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/src/tape/inventory.rs b/src/tape/inventory.rs
index d56b2144..f45cec71 100644
--- a/src/tape/inventory.rs
+++ b/src/tape/inventory.rs
@@ -40,7 +40,28 @@ use proxmox::tools::{
 };
 
 use pbs_systemd::time::compute_next_event;
-use pbs_config::{open_backup_lockfile, BackupLockGuard};
+use pbs_config::BackupLockGuard;
+
+#[cfg(not(test))]
+use pbs_config::open_backup_lockfile;
+
+#[cfg(test)]
+mod lock_mocking {
+    struct MockedLock;
+
+    impl pbs_config::BackupLockGuardTrait for MockedLock {}
+
+    pub fn open_backup_lockfile<P: AsRef<std::path::Path>>(
+        _path: P,
+        _timeout: Option<std::time::Duration>,
+        _exclusive: bool,
+    ) -> Result<pbs_config::BackupLockGuard, anyhow::Error> {
+        Ok(Box::new(MockedLock))
+    }
+}
+
+#[cfg(test)]
+use lock_mocking::*;
 
 use crate::{
     api2::types::{
-- 
2.30.2

Re: [pbs-devel] [RFC PATCH proxmox-backup 1/2] pbs-config: use trait object for the backup lock guard

[Wolfgang Bumiller]

On Fri, Sep 03, 2021 at 09:17:51AM +0200, Dominik Csapak wrote:
> instead of a fixed type. The old implementation is now
> BackupLockGuardImpl and implements the trait.
> 
> At the same time, introduce a type alias with the same name as the
> previous struct, so that the users of it do not have to change anything.
> 
> This makes it possible for us to have a different lock implementation
> for e.g. tests (where we do not actually want to lock)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-config/src/lib.rs | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/pbs-config/src/lib.rs b/pbs-config/src/lib.rs
> index 9d8c730d..604fe9d7 100644
> --- a/pbs-config/src/lib.rs
> +++ b/pbs-config/src/lib.rs
> @@ -16,7 +16,14 @@ pub fn backup_group() -> Result<nix::unistd::Group, Error> {
>      pbs_tools::sys::query_group(BACKUP_GROUP_NAME)?
>          .ok_or_else(|| format_err!("Unable to lookup '{}' group.", BACKUP_GROUP_NAME))
>  }
> -pub struct BackupLockGuard(std::fs::File);
> +
> +pub trait BackupLockGuardTrait: Send + Sync + Unpin + std::panic::UnwindSafe + std::panic::RefUnwindSafe { }
> +
> +struct BackupLockGuardImpl(std::fs::File);
> +
> +impl BackupLockGuardTrait for BackupLockGuardImpl {}
> +
> +pub type BackupLockGuard = Box<dyn BackupLockGuardTrait>;

Since the File is abstracted away in a custom type, 2 more possibilities
come to mind:

    pub struct BackupLockGuard(Option<File>);

and cfg(test) would just use `None`

or turn the File into our `Fd` type from the proxmox crate and use
`Fd::from_raw_fd(-1)` when no lock is used

That way we don't even need the trait

Re: [pbs-devel] [RFC PATCH proxmox-backup 1/2] pbs-config: use trait object for the backup lock guard

[Dominik Csapak]

On 9/6/21 12:25, Wolfgang Bumiller wrote:
> On Fri, Sep 03, 2021 at 09:17:51AM +0200, Dominik Csapak wrote:
>> instead of a fixed type. The old implementation is now
>> BackupLockGuardImpl and implements the trait.
>>
>> At the same time, introduce a type alias with the same name as the
>> previous struct, so that the users of it do not have to change anything.
>>
>> This makes it possible for us to have a different lock implementation
>> for e.g. tests (where we do not actually want to lock)
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   pbs-config/src/lib.rs | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/pbs-config/src/lib.rs b/pbs-config/src/lib.rs
>> index 9d8c730d..604fe9d7 100644
>> --- a/pbs-config/src/lib.rs
>> +++ b/pbs-config/src/lib.rs
>> @@ -16,7 +16,14 @@ pub fn backup_group() -> Result<nix::unistd::Group, Error> {
>>       pbs_tools::sys::query_group(BACKUP_GROUP_NAME)?
>>           .ok_or_else(|| format_err!("Unable to lookup '{}' group.", BACKUP_GROUP_NAME))
>>   }
>> -pub struct BackupLockGuard(std::fs::File);
>> +
>> +pub trait BackupLockGuardTrait: Send + Sync + Unpin + std::panic::UnwindSafe + std::panic::RefUnwindSafe { }
>> +
>> +struct BackupLockGuardImpl(std::fs::File);
>> +
>> +impl BackupLockGuardTrait for BackupLockGuardImpl {}
>> +
>> +pub type BackupLockGuard = Box<dyn BackupLockGuardTrait>;
> 
> Since the File is abstracted away in a custom type, 2 more possibilities
> come to mind:
> 
>      pub struct BackupLockGuard(Option<File>);
> 
> and cfg(test) would just use `None`
> 
> or turn the File into our `Fd` type from the proxmox crate and use
> `Fd::from_raw_fd(-1)` when no lock is used
> 
> That way we don't even need the trait
> 

sound ok in general, but then we have to make the field public no ?
and then we could also open a bug file too, no change in the type needed 
at all... but i thought we want to avoid making the underlying field 
public...

Re: [pbs-devel] [RFC PATCH proxmox-backup 1/2] pbs-config: use trait object for the backup lock guard

[Dominik Csapak]

[...]
>>
> 
> sound ok in general, but then we have to make the field public no ?
> and then we could also open a bug file too, no change in the type needed 
> at all... but i thought we want to avoid making the underlying field 
> public...
> 
> 

i meant a 'bogus' file here, not bug...