Re: [pbs-devel] [PATCH proxmox-backup] tape: fix 'eject-before-unload' api type

[Dominik Csapak <d.csapak@proxmox.com>]

On 12/14/23 08:46, Thomas Lamprecht wrote:
> Am 13/12/2023 um 11:11 schrieb Dominik Csapak:
>> by converting the bool into an option, otherwise having the options not
>> set at all will fail the unload while deserializing with
>> 'eject-before-unload is not optional'
>>
>> Also if we can automatically decide this in the future, we can now
>> detect if the option was explicitely set or not.
>>
>> Fixes: 66402cdc ("fix #4904: tape changer: add option to eject before unload")
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   pbs-api-types/src/tape/changer.rs | 17 ++++++++++-------
>>   src/tape/changer/mod.rs           |  2 +-
>>   2 files changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/pbs-api-types/src/tape/changer.rs b/pbs-api-types/src/tape/changer.rs
>> index e3cf27c1..9e36b12e 100644
>> --- a/pbs-api-types/src/tape/changer.rs
>> +++ b/pbs-api-types/src/tape/changer.rs
>> @@ -39,18 +39,21 @@ Import/Export, i.e. any media in those slots are considered to be
>>   .format(&ApiStringFormat::PropertyString(&SLOT_ARRAY_SCHEMA))
>>   .schema();
>>   
>> -fn is_false(b: &bool) -> bool {
>> -    !b
>> -}
>> -
>> -#[api]
>> +#[api(
>> +    properties: {
>> +        "eject-before-unload": {
>> +            optional: true,
>> +            default: false,
>> +        },
>> +    },
>> +)]
>>   #[derive(Serialize, Deserialize)]
>>   #[serde(rename_all = "kebab-case")]
>>   /// Options for Changers
>>   pub struct ChangerOptions {
> 
> semi-related to your change, but as I talked with Dietmar off-list about this and
> thus checked it out a bit more closely:
> 
> As this is already on the changer section type it might make more sense to just have
> the "eject-before-unload" option there directly though.
> 
> As having a property-string here seems a bit odd, we mostly use them if we want to
> configure more things at once on a single (sub)-subject, like network for VMs, but
> if you'd have this as "plain" option for the change section-type, it would already
> affect the correct subject.
> 
> If we get many more such options in the future, we can always move them into a
> property-string grouping for a next major release, but tbh. I would be slightly
> surprised if it's more than a handful such new options in the next decade.
> And even if, as long as they affect the changer, not a sub-subject, it can be
> still fine to have any such new option also standalone on the top-level
> 


mhmm i modeled it after the 'tuning' options of the datastore.
maybe it was named badly, and the 'options' should be replaced by 
'quirks' (as in, changer quirks that only some people need, like
the datastore tuning options)

but if you both want a more straight forward option directly in
the changer config, then it's also fine with me (i just
did not want to pollute the changer config with rather
specific quirk workarounds, and I doubt this is
the last of them, even though we don't see such things often)

just tell me if i should rename it to something else (like quirks)
or if i should put it directly in the changer config, and i'll send
the patches for it + docs update