[pbs-devel] [PATCH proxmox-backup v2 1/2] pbs-config: add 'create_mocked_lock' helper

[pbs-devel] [PATCH proxmox-backup v2 1/2] pbs-config: add 'create_mocked_lock' helper

[Dominik Csapak]

by making the field an option and making it None in the mocked case
this function is only intended for testing and hidden from the docs

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* mark function unsafe, so that it is even more clear that this is not
  intended for 'normal' code
 pbs-config/src/lib.rs | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/pbs-config/src/lib.rs b/pbs-config/src/lib.rs
index 29811164..bd5430a3 100644
--- a/pbs-config/src/lib.rs
+++ b/pbs-config/src/lib.rs
@@ -18,7 +18,13 @@ 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 struct BackupLockGuard(Option<std::fs::File>);
+
+#[doc(hidden)]
+/// Note: do not use for production code, this is only intended for tests
+pub unsafe fn create_mocked_lock() -> BackupLockGuard {
+    BackupLockGuard(None)
+}
 
 /// Open or create a lock file owned by user "backup" and lock it.
 ///
@@ -41,7 +47,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(BackupLockGuard(Some(file)))
 }
 
 /// Atomically write data to file owned by "root:backup" with permission "0640"
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 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 implementing 'open_backup_lockfile' with
'create_mocked_lock'

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* put unsafe block around 'create_mocked_lock'
 src/tape/inventory.rs | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/tape/inventory.rs b/src/tape/inventory.rs
index d56b2144..93d72057 100644
--- a/src/tape/inventory.rs
+++ b/src/tape/inventory.rs
@@ -40,7 +40,20 @@ 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)]
+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(unsafe { pbs_config::create_mocked_lock() })
+}
+
 
 use crate::{
     api2::types::{
-- 
2.30.2

[pbs-devel] applied: [PATCH proxmox-backup v2 1/2] pbs-config: add 'create_mocked_lock' helper

[Wolfgang Bumiller]

applied both patches

On Mon, Sep 06, 2021 at 05:00:25PM +0200, Dominik Csapak wrote:
> by making the field an option and making it None in the mocked case
> this function is only intended for testing and hidden from the docs
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v1:
> * mark function unsafe, so that it is even more clear that this is not
>   intended for 'normal' code
>  pbs-config/src/lib.rs | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/pbs-config/src/lib.rs b/pbs-config/src/lib.rs
> index 29811164..bd5430a3 100644
> --- a/pbs-config/src/lib.rs
> +++ b/pbs-config/src/lib.rs
> @@ -18,7 +18,13 @@ 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 struct BackupLockGuard(Option<std::fs::File>);
> +
> +#[doc(hidden)]
> +/// Note: do not use for production code, this is only intended for tests
> +pub unsafe fn create_mocked_lock() -> BackupLockGuard {
> +    BackupLockGuard(None)
> +}
>  
>  /// Open or create a lock file owned by user "backup" and lock it.
>  ///
> @@ -41,7 +47,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(BackupLockGuard(Some(file)))
>  }
>  
>  /// Atomically write data to file owned by "root:backup" with permission "0640"
> -- 
> 2.30.2

Re: [pbs-devel] applied: [PATCH proxmox-backup v2 1/2] pbs-config: add 'create_mocked_lock' helper

[Dietmar Maurer]

I really do not like this solution :-(

> On 09/07/2021 9:05 AM Wolfgang Bumiller <w.bumiller@proxmox.com> wrote:
> 
>  
> applied both patches
> 
> On Mon, Sep 06, 2021 at 05:00:25PM +0200, Dominik Csapak wrote:
> > by making the field an option and making it None in the mocked case
> > this function is only intended for testing and hidden from the docs
> > 
> > Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> > ---
> > changes from v1:
> > * mark function unsafe, so that it is even more clear that this is not
> >   intended for 'normal' code
> >  pbs-config/src/lib.rs | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/pbs-config/src/lib.rs b/pbs-config/src/lib.rs
> > index 29811164..bd5430a3 100644
> > --- a/pbs-config/src/lib.rs
> > +++ b/pbs-config/src/lib.rs
> > @@ -18,7 +18,13 @@ 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 struct BackupLockGuard(Option<std::fs::File>);
> > +
> > +#[doc(hidden)]
> > +/// Note: do not use for production code, this is only intended for tests
> > +pub unsafe fn create_mocked_lock() -> BackupLockGuard {
> > +    BackupLockGuard(None)
> > +}
> >  
> >  /// Open or create a lock file owned by user "backup" and lock it.
> >  ///
> > @@ -41,7 +47,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(BackupLockGuard(Some(file)))
> >  }
> >  
> >  /// Atomically write data to file owned by "root:backup" with permission "0640"
> > -- 
> > 2.30.2
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] applied: [PATCH proxmox-backup v2 1/2] pbs-config: add 'create_mocked_lock' helper

[Dominik Csapak]

On 9/7/21 09:49, Dietmar Maurer wrote:
> I really do not like this solution :-(
> 

was the trait approach better ?

since the only remaining way i can can currently think of is to
workaround the locks everywhere in src/tape/*.rs

Re: [pbs-devel] applied: [PATCH proxmox-backup v2 1/2] pbs-config: add 'create_mocked_lock' helper

[Dietmar Maurer]

> On 9/7/21 09:49, Dietmar Maurer wrote:
> > I really do not like this solution :-(
> > 
> 
> was the trait approach better ?

> 
no, that was worse

> since the only remaining way i can can currently think of is to
> workaround the locks everywhere in src/tape/*.rs

I am also out of ideas ...

At least the tests works again.