[pbs-devel] [PATCH proxmox-backup] api: enhance directory existence check

[pbs-devel] [PATCH proxmox-backup] api: enhance directory existence check

[Markus Frank]

Instead of just checking the existence of the path, the code now also checks
whether the directory is empty, continues if it is, and aborts if it is not.

Previously, if a directory were deleted and a directory with the same name
would be created, the old check prevented the creation even though the
directory could be used as a mount point.

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 src/api2/node/disks/directory.rs | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index 5e1cb124..cd7b44a3 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -155,13 +155,17 @@ pub fn create_datastore_disk(
 
     let mount_point = format!("{}{}", BASE_MOUNT_DIR, &name);
 
-    // check if the default path does exist already and bail if it does
+    // check if the default path does exist already
+    // and bail if it does and is not empty
     let default_path = std::path::PathBuf::from(&mount_point);
 
     match std::fs::metadata(&default_path) {
         Err(_) => {} // path does not exist
         Ok(_) => {
-            bail!("path {:?} already exists", default_path);
+            let is_empty = default_path.read_dir()?.next().is_none();
+            if !is_empty {
+                bail!("path {:?} already exists and is not empty", default_path);
+            }
         }
     }
 
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox-backup] api: enhance directory existence check

[Lukas Wagner]

On 11/29/23 15:08, Markus Frank wrote:
> Instead of just checking the existence of the path, the code now also checks
> whether the directory is empty, continues if it is, and aborts if it is not.
> 
> Previously, if a directory were deleted and a directory with the same name
> would be created, the old check prevented the creation even though the
> directory could be used as a mount point.


But that does not detect if another (empty) filesystem is already 
mounted at that directory, right?

> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>   src/api2/node/disks/directory.rs | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
> index 5e1cb124..cd7b44a3 100644
> --- a/src/api2/node/disks/directory.rs
> +++ b/src/api2/node/disks/directory.rs
> @@ -155,13 +155,17 @@ pub fn create_datastore_disk(
>   
>       let mount_point = format!("{}{}", BASE_MOUNT_DIR, &name);
>   
> -    // check if the default path does exist already and bail if it does
> +    // check if the default path does exist already
> +    // and bail if it does and is not empty
>       let default_path = std::path::PathBuf::from(&mount_point);
>   
>       match std::fs::metadata(&default_path) {
>           Err(_) => {} // path does not exist
>           Ok(_) => {
> -            bail!("path {:?} already exists", default_path);
> +            let is_empty = default_path.read_dir()?.next().is_none();
> +            if !is_empty {
> +                bail!("path {:?} already exists and is not empty", default_path);
You can inline 'default_path' here: {default_path:?}

> +            }
>           }
>       }
>   

Also, I guess you could change the 'match' to a
'if std::fs::metadata(...).is_ok()', since we ignore the enclosed values 
anyway.

-- 
- Lukas

Re: [pbs-devel] [PATCH proxmox-backup] api: enhance directory existence check

[Christian Ebner]

> On 29.11.2023 15:51 CET Lukas Wagner <l.wagner@proxmox.com> wrote:
> 
> 
> 
> But that does not detect if another (empty) filesystem is already 
> mounted at that directory, right?

To add to Lukas response, you could compare the `st_dev` of parent and mountpoint
to check if there already if a different filesystem mounted, something like:

use std::os::linux::fs::MetadataExt;

...

match std::fs::metadata(&default_path) {
     Err(_) => {} // path does not exist
     Ok(stat) => {
         let basedir_dev = std::fs::metadata(BASE_MOUNT_DIR)?.st_dev();
         if stat.st_dev != basedir_dev {
             bail!("path {default_path:?} already exists and is mountpoint");
         }
         ...
    }
}