From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 385F5BA499 for ; Thu, 14 Dec 2023 08:52:26 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 117FD13CA2 for ; Thu, 14 Dec 2023 08:52:26 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 14 Dec 2023 08:52:25 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 347FF4737C for ; Thu, 14 Dec 2023 08:52:25 +0100 (CET) Message-ID: Date: Thu, 14 Dec 2023 08:52:24 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Thomas Lamprecht , Proxmox Backup Server development discussion References: <20231213101112.76140-1-d.csapak@proxmox.com> From: Dominik Csapak In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.020 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH proxmox-backup] tape: fix 'eject-before-unload' api type X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Dec 2023 07:52:26 -0000 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 >> --- >> 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