[pbs-devel] [PATCH proxmox-backup 0/2] api: disks: directory: fail if mount unit already exists

[pbs-devel] [PATCH proxmox-backup 0/2] api: disks: directory: fail if mount unit already exists

[Fiona Ebner]

Protect against overriding a pre-existing mount unit during creation
of a directory (datastore). The unit might belong to an existing
datastore.

Fiona Ebner (2):
  api: disks: directory: factor out helper for mount unit path
  api: disks: directory: fail if mount unit already exists

 src/api2/node/disks/directory.rs | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 1/2] api: disks: directory: factor out helper for mount unit path

[Fiona Ebner]

In preparation to check for a pre-existing mount unit.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/api2/node/disks/directory.rs | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index 6a76dd5a..7bdf0111 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -324,16 +324,23 @@ pub const ROUTER: Router = Router::new()
     .post(&API_METHOD_CREATE_DATASTORE_DISK)
     .match_all("name", &ITEM_ROUTER);
 
+fn datastore_mount_unit_path_info(mount_point: &str) -> (String, String) {
+    let mut mount_unit_name = proxmox_systemd::escape_unit(mount_point, true);
+    mount_unit_name.push_str(".mount");
+
+    (
+        format!("/etc/systemd/system/{}", mount_unit_name),
+        mount_unit_name,
+    )
+}
+
 fn create_datastore_mount_unit(
     datastore_name: &str,
     mount_point: &str,
     fs_type: FileSystemType,
     what: &str,
 ) -> Result<String, Error> {
-    let mut mount_unit_name = proxmox_systemd::escape_unit(mount_point, true);
-    mount_unit_name.push_str(".mount");
-
-    let mount_unit_path = format!("/etc/systemd/system/{}", mount_unit_name);
+    let (mount_unit_path, mount_unit_name) = datastore_mount_unit_path_info(mount_point);
 
     let unit = SystemdUnitSection {
         Description: format!(
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 2/2] api: disks: directory: fail if mount unit already exists

[Fiona Ebner]

Without this check, if a mount unit is present, but the file system is
not mounted, it will just get overwritten. The unit might belong to an
existing datastore.

There already is a check against a duplicate datastore, but only after
the mount unit is already overwritten and having the add-datastore
flag present is not a precondition to trigger the issue.

The check is done even if the newly created directory datastore is
removable. While in that case, the mount unit is not overwritten, the
conflict for the mount point is still present, so it is nice to fail
early.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/api2/node/disks/directory.rs | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index 7bdf0111..d4690d45 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -204,6 +204,11 @@ pub fn create_datastore_disk(
         }
     }
 
+    let (mount_unit_path, _) = datastore_mount_unit_path_info(&mount_point);
+    if std::path::PathBuf::from(&mount_unit_path).exists() {
+        bail!("systemd mount unit {mount_unit_path:?} already exists");
+    }
+
     let upid_str = WorkerTask::new_thread(
         "dircreate",
         Some(name.clone()),
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 1/2] api: disks: directory: factor out helper for mount unit path

[Shannon Sterz]

On Wed Nov 27, 2024 at 4:06 PM CET, Fiona Ebner wrote:
> In preparation to check for a pre-existing mount unit.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  src/api2/node/disks/directory.rs | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
> index 6a76dd5a..7bdf0111 100644
> --- a/src/api2/node/disks/directory.rs
> +++ b/src/api2/node/disks/directory.rs
> @@ -324,16 +324,23 @@ pub const ROUTER: Router = Router::new()
>      .post(&API_METHOD_CREATE_DATASTORE_DISK)
>      .match_all("name", &ITEM_ROUTER);
>
> +fn datastore_mount_unit_path_info(mount_point: &str) -> (String, String) {
> +    let mut mount_unit_name = proxmox_systemd::escape_unit(mount_point, true);
> +    mount_unit_name.push_str(".mount");
> +
> +    (
> +        format!("/etc/systemd/system/{}", mount_unit_name),

nit, this could be:

```rs
format!("/etc/systemd/system/{mount_unit_name}"),
```

> +        mount_unit_name,
> +    )
> +}
> +
>  fn create_datastore_mount_unit(
>      datastore_name: &str,
>      mount_point: &str,
>      fs_type: FileSystemType,
>      what: &str,
>  ) -> Result<String, Error> {
> -    let mut mount_unit_name = proxmox_systemd::escape_unit(mount_point, true);
> -    mount_unit_name.push_str(".mount");
> -
> -    let mount_unit_path = format!("/etc/systemd/system/{}", mount_unit_name);
> +    let (mount_unit_path, mount_unit_name) = datastore_mount_unit_path_info(mount_point);
>
>      let unit = SystemdUnitSection {
>          Description: format!(

other than that consider this:

Reviewed-by: Shannon Sterz <s.sterz@proxmox.com>




_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 2/2] api: disks: directory: fail if mount unit already exists

[Shannon Sterz]

On Wed Nov 27, 2024 at 4:06 PM CET, Fiona Ebner wrote:
> Without this check, if a mount unit is present, but the file system is
> not mounted, it will just get overwritten. The unit might belong to an
> existing datastore.
>
> There already is a check against a duplicate datastore, but only after
> the mount unit is already overwritten and having the add-datastore
> flag present is not a precondition to trigger the issue.
>
> The check is done even if the newly created directory datastore is
> removable. While in that case, the mount unit is not overwritten, the
> conflict for the mount point is still present, so it is nice to fail
> early.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  src/api2/node/disks/directory.rs | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
> index 7bdf0111..d4690d45 100644
> --- a/src/api2/node/disks/directory.rs
> +++ b/src/api2/node/disks/directory.rs
> @@ -204,6 +204,11 @@ pub fn create_datastore_disk(
>          }
>      }
>
> +    let (mount_unit_path, _) = datastore_mount_unit_path_info(&mount_point);
> +    if std::path::PathBuf::from(&mount_unit_path).exists() {
> +        bail!("systemd mount unit {mount_unit_path:?} already exists");

nit: relying on the `Debug` trait here to quote the string feels a bit..
hacky to me? maybe just:

```rs
bail!("systemd mount unit \"{mount_unit_path}\" already exists");
```

or use single quotes as we do elsewhere.

> +    }
> +
>      let upid_str = WorkerTask::new_thread(
>          "dircreate",
>          Some(name.clone()),

Otherwise:

Reviewed-by: Shannon Sterz <s.sterz@proxmox.com>


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 2/2] api: disks: directory: fail if mount unit already exists

[Thomas Lamprecht]

Am 27.11.24 um 16:23 schrieb Shannon Sterz:
> On Wed Nov 27, 2024 at 4:06 PM CET, Fiona Ebner wrote:
>> +    if std::path::PathBuf::from(&mount_unit_path).exists() {
>> +        bail!("systemd mount unit {mount_unit_path:?} already exists");
> 
> nit: relying on the `Debug` trait here to quote the string feels a bit..
> hacky to me? maybe just:
> 
> ```rs
> bail!("systemd mount unit \"{mount_unit_path}\" already exists");
> ```
> 
> or use single quotes as we do elsewhere.
> 

FWIW, we (mis)use this semi-frequently already, it might not be the cleanest
thing but works out OK; I'm fine either way, just wanted to mention that if
this really has some issue, or is considered non-idiomatic, then a tree-wide
clean-up would be warranted, otherwise its usage will probably grow due to
copying existing code as base for new dev work.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 2/2] api: disks: directory: fail if mount unit already exists

[Fiona Ebner]

Am 27.11.24 um 16:26 schrieb Thomas Lamprecht:
> Am 27.11.24 um 16:23 schrieb Shannon Sterz:
>> On Wed Nov 27, 2024 at 4:06 PM CET, Fiona Ebner wrote:
>>> +    if std::path::PathBuf::from(&mount_unit_path).exists() {
>>> +        bail!("systemd mount unit {mount_unit_path:?} already exists");
>>
>> nit: relying on the `Debug` trait here to quote the string feels a bit..
>> hacky to me? maybe just:
>>
>> ```rs
>> bail!("systemd mount unit \"{mount_unit_path}\" already exists");
>> ```
>>
>> or use single quotes as we do elsewhere.
>>
> 
> FWIW, we (mis)use this semi-frequently already, it might not be the cleanest
> thing but works out OK; I'm fine either way, just wanted to mention that if
> this really has some issue, or is considered non-idiomatic, then a tree-wide
> clean-up would be warranted, otherwise its usage will probably grow due to
> copying existing code as base for new dev work.
> 

I did copy the pattern from surrounding code 0:)

Sent a v2 addressing Shannon's comments, thanks!
https://lore.proxmox.com/pbs-devel/20241127160637.131088-1-f.ebner@proxmox.com/T/


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel