[pbs-devel] [PATCH proxmox-backup 1/2] backup/snapshot reader: improve error message for ENOENT

[pbs-devel] [PATCH proxmox-backup 1/2] backup/snapshot reader: improve error message for ENOENT

[Fabian Grünbichler]

instead of

 Error: unable to open snapshot directory "/full/path/to/snapshot" for locking - ENOENT: No such file or directory

this will now print

 Error: Snapshot vm/800/2023-01-16T12:28:11Z does not exist.

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

Notes:
    we could also include the datastore/namespace, although we are not that
    consistent in general in that regard, and the caller should technically
    know since they provided it ;).

 pbs-datastore/src/snapshot_reader.rs | 4 ++++
 src/api2/reader/mod.rs               | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/pbs-datastore/src/snapshot_reader.rs b/pbs-datastore/src/snapshot_reader.rs
index 899c3bce..ec7a48e5 100644
--- a/pbs-datastore/src/snapshot_reader.rs
+++ b/pbs-datastore/src/snapshot_reader.rs
@@ -41,6 +41,10 @@ impl SnapshotReader {
         let datastore = snapshot.datastore();
         let snapshot_path = snapshot.full_path();
 
+        if !snapshot_path.exists() {
+            bail!("snapshot {} does not exist!", snapshot.dir());
+        }
+
         let locked_dir =
             lock_dir_noblock_shared(&snapshot_path, "snapshot", "locked by another operation")?;
 
diff --git a/src/api2/reader/mod.rs b/src/api2/reader/mod.rs
index e2a10da3..1ac4ac40 100644
--- a/src/api2/reader/mod.rs
+++ b/src/api2/reader/mod.rs
@@ -125,6 +125,10 @@ fn upgrade_to_backup_reader_protocol(
             }
         }
 
+        if !backup_dir.full_path().exists() {
+            bail!("snapshot {} does not exist.", backup_dir.dir());
+        }
+
         let _guard = lock_dir_noblock_shared(
             &backup_dir.full_path(),
             "snapshot",
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 2/2] verify/protect: improve error on disappearing snapshots

[Fabian Grünbichler]

or clients passing in a non-existent snapshot.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 pbs-datastore/src/datastore.rs |  4 ++++
 src/backup/verify.rs           | 10 ++++++++++
 2 files changed, 14 insertions(+)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 6f37e886..bf951305 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1180,6 +1180,10 @@ impl DataStore {
     pub fn update_protection(&self, backup_dir: &BackupDir, protection: bool) -> Result<(), Error> {
         let full_path = backup_dir.full_path();
 
+        if !full_path.exists() {
+            bail!("snapshot {} does not exist!", backup_dir.dir());
+        }
+
         let _guard = lock_dir_noblock(&full_path, "snapshot", "possibly running or in use")?;
 
         let protected_path = backup_dir.protected_file();
diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index 3984e28d..c972e532 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -328,6 +328,16 @@ pub fn verify_backup_dir(
     upid: UPID,
     filter: Option<&dyn Fn(&BackupManifest) -> bool>,
 ) -> Result<bool, Error> {
+    if !backup_dir.full_path().exists() {
+        task_log!(
+            verify_worker.worker,
+            "SKIPPED: verify {}:{} - snapshot does not exist (anymore).",
+            verify_worker.datastore.name(),
+            backup_dir.dir(),
+        );
+        return Ok(true);
+    }
+
     let snap_lock = lock_dir_noblock_shared(
         &backup_dir.full_path(),
         "snapshot",
-- 
2.30.2

[pbs-devel] applied-series: [PATCH proxmox-backup 1/2] backup/snapshot reader: improve error message for ENOENT

[Wolfgang Bumiller]

While I really do not like `.exists()` on a path that is used right
afterwards and our "helpers" there need to switch to `io::Error` instead
of useless `anyhow::Error` strings...
expecting our locking to change and the paths don't matching up
afterwards this series should be fine this way, too.

applied, thanks

On Thu, Feb 02, 2023 at 02:04:23PM +0100, Fabian Grünbichler wrote:
> instead of
> 
>  Error: unable to open snapshot directory "/full/path/to/snapshot" for locking - ENOENT: No such file or directory
> 
> this will now print
> 
>  Error: Snapshot vm/800/2023-01-16T12:28:11Z does not exist.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>     we could also include the datastore/namespace, although we are not that
>     consistent in general in that regard, and the caller should technically
>     know since they provided it ;).
> 
>  pbs-datastore/src/snapshot_reader.rs | 4 ++++
>  src/api2/reader/mod.rs               | 4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/pbs-datastore/src/snapshot_reader.rs b/pbs-datastore/src/snapshot_reader.rs
> index 899c3bce..ec7a48e5 100644
> --- a/pbs-datastore/src/snapshot_reader.rs
> +++ b/pbs-datastore/src/snapshot_reader.rs
> @@ -41,6 +41,10 @@ impl SnapshotReader {
>          let datastore = snapshot.datastore();
>          let snapshot_path = snapshot.full_path();
>  
> +        if !snapshot_path.exists() {
> +            bail!("snapshot {} does not exist!", snapshot.dir());
> +        }
> +
>          let locked_dir =
>              lock_dir_noblock_shared(&snapshot_path, "snapshot", "locked by another operation")?;
>  
> diff --git a/src/api2/reader/mod.rs b/src/api2/reader/mod.rs
> index e2a10da3..1ac4ac40 100644
> --- a/src/api2/reader/mod.rs
> +++ b/src/api2/reader/mod.rs
> @@ -125,6 +125,10 @@ fn upgrade_to_backup_reader_protocol(
>              }
>          }
>  
> +        if !backup_dir.full_path().exists() {
> +            bail!("snapshot {} does not exist.", backup_dir.dir());
> +        }
> +
>          let _guard = lock_dir_noblock_shared(
>              &backup_dir.full_path(),
>              "snapshot",
> -- 
> 2.30.2