[pbs-devel] [PATCH proxmox-backup] api: tape: don't allow overwriting of ids in changer/drive config

[pbs-devel] [PATCH proxmox-backup] api: tape: don't allow overwriting of ids in changer/drive config

[Dominik Csapak]

by checking the whole section config for an existing id, not only the
ones of the given type.

This prevents creation of a drive config with the same name as an
existing changer and vice versa.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/config/changer.rs | 8 ++++----
 src/api2/config/drive.rs   | 7 ++++---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/api2/config/changer.rs b/src/api2/config/changer.rs
index db0ea14a..91286697 100644
--- a/src/api2/config/changer.rs
+++ b/src/api2/config/changer.rs
@@ -33,6 +33,10 @@ pub fn create_changer(config: ScsiTapeChanger) -> Result<(), Error> {
 
     let (mut section_config, _digest) = pbs_config::drive::config()?;
 
+    if section_config.sections.get(&config.name).is_some() {
+        param_bail!("name", "Entry '{}' already exists", config.name);
+    }
+
     let linux_changers = linux_tape_changer_list();
 
     check_drive_path(&linux_changers, &config.path)?;
@@ -40,10 +44,6 @@ pub fn create_changer(config: ScsiTapeChanger) -> Result<(), Error> {
     let existing: Vec<ScsiTapeChanger> = section_config.convert_to_typed_array("changer")?;
 
     for changer in existing {
-        if changer.name == config.name {
-            param_bail!("name", "Entry '{}' already exists", config.name);
-        }
-
         if changer.path == config.path {
             param_bail!(
                 "path",
diff --git a/src/api2/config/drive.rs b/src/api2/config/drive.rs
index 02589aaf..cf620aaf 100644
--- a/src/api2/config/drive.rs
+++ b/src/api2/config/drive.rs
@@ -34,6 +34,10 @@ pub fn create_drive(config: LtoTapeDrive) -> Result<(), Error> {
 
     let (mut section_config, _digest) = pbs_config::drive::config()?;
 
+    if section_config.sections.get(&config.name).is_some() {
+        param_bail!("name", "Entry '{}' already exists", config.name);
+    }
+
     let lto_drives = lto_tape_device_list();
 
     check_drive_path(&lto_drives, &config.path)?;
@@ -41,9 +45,6 @@ pub fn create_drive(config: LtoTapeDrive) -> Result<(), Error> {
     let existing: Vec<LtoTapeDrive> = section_config.convert_to_typed_array("lto")?;
 
     for drive in existing {
-        if drive.name == config.name {
-            param_bail!("name", "Entry '{}' already exists", config.name);
-        }
         if drive.path == config.path {
             param_bail!(
                 "path",
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox-backup] api: tape: don't allow overwriting of ids in changer/drive config

[Thomas Lamprecht]

Am 06/03/2024 um 09:47 schrieb Dominik Csapak:
> by checking the whole section config for an existing id, not only the
> ones of the given type.
> 
> This prevents creation of a drive config with the same name as an
> existing changer and vice versa.

Ok, but why is that bad? Just confusion potential, does something
break as we only use the ID without the section-key later on, ...?

Describing the effects/background, even if rather obvious, would
be still appreciated.

Re: [pbs-devel] [PATCH proxmox-backup] api: tape: don't allow overwriting of ids in changer/drive config

[Dominik Csapak]

On 3/6/24 09:56, Thomas Lamprecht wrote:
> Am 06/03/2024 um 09:47 schrieb Dominik Csapak:
>> by checking the whole section config for an existing id, not only the
>> ones of the given type.
>>
>> This prevents creation of a drive config with the same name as an
>> existing changer and vice versa.
> 
> Ok, but why is that bad? Just confusion potential, does something
> break as we only use the ID without the section-key later on, ...?
> 
> Describing the effects/background, even if rather obvious, would
> be still appreciated.

ah yes, sorry

first, it's really unexpected that creating a changer 'foo' deletes the existing drive 'foo'
(or vice versa).

but it also breaks our assumptions a bit, for example:

i create a changer 'foo'
then i create the drive 'foo' and select changer 'foo' as it's changer

this works, but afterwards i don't have the changer anymore in the config
but the drive still references that

when i now select the changer 'foo' in the navigation, i'm
greeted with the error:

`got unexpected type 'lto' for changer 'foo'`

should i send a  v2 with an updated commit message?

Re: [pbs-devel] [PATCH proxmox-backup] api: tape: don't allow overwriting of ids in changer/drive config

[Thomas Lamprecht]

Am 06/03/2024 um 10:09 schrieb Dominik Csapak:
> On 3/6/24 09:56, Thomas Lamprecht wrote:
>> Am 06/03/2024 um 09:47 schrieb Dominik Csapak:
>>> by checking the whole section config for an existing id, not only the
>>> ones of the given type.
>>>
>>> This prevents creation of a drive config with the same name as an
>>> existing changer and vice versa.
>>
>> Ok, but why is that bad? Just confusion potential, does something
>> break as we only use the ID without the section-key later on, ...?
>>
>> Describing the effects/background, even if rather obvious, would
>> be still appreciated.
> 
> ah yes, sorry
> 
> first, it's really unexpected that creating a changer 'foo' deletes the existing drive 'foo'
> (or vice versa).
> 
> but it also breaks our assumptions a bit, for example:
> 
> i create a changer 'foo'
> then i create the drive 'foo' and select changer 'foo' as it's changer
> 
> this works, but afterwards i don't have the changer anymore in the config
> but the drive still references that
> 
> when i now select the changer 'foo' in the navigation, i'm
> greeted with the error:
> 
> `got unexpected type 'lto' for changer 'foo'`

Ok, yeah, disallowing this explicitly sounds sensible enough and
easier compared to fixing support for changer and drive having their
own ID namespace.

> 
> should i send a  v2 with an updated commit message?
> 

yes please, as going in the other direction (fully support this) is
an option it would be good to encode that we actively decided against
it (as that's to cheap to not do even if we never need to rethink
this again).