* [pbs-devel] [PATCH proxmox-backup 0/2] api: disks: directory: fail if mount unit already exists @ 2024-11-27 15:06 Fiona Ebner 2024-11-27 15:06 ` [pbs-devel] [PATCH proxmox-backup 1/2] api: disks: directory: factor out helper for mount unit path Fiona Ebner 2024-11-27 15:06 ` [pbs-devel] [PATCH proxmox-backup 2/2] api: disks: directory: fail if mount unit already exists Fiona Ebner 0 siblings, 2 replies; 7+ messages in thread From: Fiona Ebner @ 2024-11-27 15:06 UTC (permalink / raw) To: pbs-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 1/2] api: disks: directory: factor out helper for mount unit path 2024-11-27 15:06 [pbs-devel] [PATCH proxmox-backup 0/2] api: disks: directory: fail if mount unit already exists Fiona Ebner @ 2024-11-27 15:06 ` Fiona Ebner 2024-11-27 15:21 ` Shannon Sterz 2024-11-27 15:06 ` [pbs-devel] [PATCH proxmox-backup 2/2] api: disks: directory: fail if mount unit already exists Fiona Ebner 1 sibling, 1 reply; 7+ messages in thread From: Fiona Ebner @ 2024-11-27 15:06 UTC (permalink / raw) To: pbs-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] api: disks: directory: factor out helper for mount unit path 2024-11-27 15:06 ` [pbs-devel] [PATCH proxmox-backup 1/2] api: disks: directory: factor out helper for mount unit path Fiona Ebner @ 2024-11-27 15:21 ` Shannon Sterz 0 siblings, 0 replies; 7+ messages in thread From: Shannon Sterz @ 2024-11-27 15:21 UTC (permalink / raw) To: Proxmox Backup Server development discussion 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 2/2] api: disks: directory: fail if mount unit already exists 2024-11-27 15:06 [pbs-devel] [PATCH proxmox-backup 0/2] api: disks: directory: fail if mount unit already exists Fiona Ebner 2024-11-27 15:06 ` [pbs-devel] [PATCH proxmox-backup 1/2] api: disks: directory: factor out helper for mount unit path Fiona Ebner @ 2024-11-27 15:06 ` Fiona Ebner 2024-11-27 15:23 ` Shannon Sterz 1 sibling, 1 reply; 7+ messages in thread From: Fiona Ebner @ 2024-11-27 15:06 UTC (permalink / raw) To: pbs-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 2/2] api: disks: directory: fail if mount unit already exists 2024-11-27 15:06 ` [pbs-devel] [PATCH proxmox-backup 2/2] api: disks: directory: fail if mount unit already exists Fiona Ebner @ 2024-11-27 15:23 ` Shannon Sterz 2024-11-27 15:26 ` Thomas Lamprecht 0 siblings, 1 reply; 7+ messages in thread From: Shannon Sterz @ 2024-11-27 15:23 UTC (permalink / raw) To: Proxmox Backup Server development discussion 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 2/2] api: disks: directory: fail if mount unit already exists 2024-11-27 15:23 ` Shannon Sterz @ 2024-11-27 15:26 ` Thomas Lamprecht 2024-11-27 16:08 ` Fiona Ebner 0 siblings, 1 reply; 7+ messages in thread From: Thomas Lamprecht @ 2024-11-27 15:26 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Shannon Sterz 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 2/2] api: disks: directory: fail if mount unit already exists 2024-11-27 15:26 ` Thomas Lamprecht @ 2024-11-27 16:08 ` Fiona Ebner 0 siblings, 0 replies; 7+ messages in thread From: Fiona Ebner @ 2024-11-27 16:08 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Thomas Lamprecht, Shannon Sterz 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-11-27 16:08 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-11-27 15:06 [pbs-devel] [PATCH proxmox-backup 0/2] api: disks: directory: fail if mount unit already exists Fiona Ebner 2024-11-27 15:06 ` [pbs-devel] [PATCH proxmox-backup 1/2] api: disks: directory: factor out helper for mount unit path Fiona Ebner 2024-11-27 15:21 ` Shannon Sterz 2024-11-27 15:06 ` [pbs-devel] [PATCH proxmox-backup 2/2] api: disks: directory: fail if mount unit already exists Fiona Ebner 2024-11-27 15:23 ` Shannon Sterz 2024-11-27 15:26 ` Thomas Lamprecht 2024-11-27 16:08 ` Fiona Ebner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox