Re: [PATCH proxmox 2/5] pbs-api-types: use match statement for maintenance mode check

[Robert Obkircher <r.obkircher@proxmox.com>]

On 03.06.26 12:35, Christian Ebner wrote:
> only one question
>
> On 6/2/26 3:00 PM, Robert Obkircher wrote:
>> Use an exhaustive match statement to be more explicit and to draw
>> attention when new variants are added.
>>
>> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
>
> Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
>
>> ---
>>   pbs-api-types/src/maintenance.rs | 27 ++++++++++++---------------
>>   1 file changed, 12 insertions(+), 15 deletions(-)
>>
>> diff --git a/pbs-api-types/src/maintenance.rs
>> b/pbs-api-types/src/maintenance.rs
>> index 2adb5d84..7def498b 100644
>> --- a/pbs-api-types/src/maintenance.rs
>> +++ b/pbs-api-types/src/maintenance.rs
>> @@ -94,25 +94,22 @@ impl MaintenanceMode {
>>       }
>>         pub fn check(&self, operation: Operation) -> Result<(),
>> Error> {
>> -        if self.ty == MaintenanceType::Delete {
>> -            bail!("datastore is being deleted");
>> -        }
>> -
>>           let message =
>> percent_encoding::percent_decode_str(self.message.as_deref().unwrap_or(""))
>>               .decode_utf8()
>>               .unwrap_or(Cow::Borrowed(""));
>
> question: should we also avoid decoding the message if not using it?
> Maybe by defining a closure for it? 

This should be fairly cheap in the common case where self.message is
None.

The bigger issue is that the message is never percent-encoded in the
first place. I'd also avoid adding ": " if there is no message. I was
going to address this in a v2 for [1] but I can also add a patch to
this series.

[1]
https://lore.proxmox.com/pbs-devel/50fe57b6-d7d4-40a1-9e17-bc904ea30cdd@proxmox.com/

>
>>   -        if Operation::Lookup == operation {
>> -            return Ok(());
>> -        } else if self.ty == MaintenanceType::Unmount {
>> -            bail!("datastore is being unmounted");
>> -        } else if self.ty == MaintenanceType::Offline {
>> -            bail!("offline maintenance mode: {}", message);
>> -        } else if self.ty == MaintenanceType::S3Refresh {
>> -            bail!("S3 refresh maintenance mode: {}", message);
>> -        } else if self.ty == MaintenanceType::ReadOnly &&
>> Operation::Write == operation {
>> -            bail!("read-only maintenance mode: {}", message);
>> +        match (self.ty, operation) {
>> +            (MaintenanceType::Delete, _) => bail!("datastore is
>> being deleted"),
>> +            (_, Operation::Lookup) => Ok(()),
>> +            (MaintenanceType::Unmount, _) => bail!("datastore is
>> being unmounted"),
>> +            (MaintenanceType::Offline, _) => bail!("offline
>> maintenance mode: {message}"),
>> +            (MaintenanceType::S3Refresh, _) => bail!("S3 refresh
>> maintenance mode: {message}"),
>> +            (MaintenanceType::ReadOnly, Operation::Read) => Ok(()),
>> +            (MaintenanceType::ReadOnly, Operation::Write) => {
>> +                bail!("read-only maintenance mode: {message}")
>> +            }
>> +            #[cfg(feature = "enum-fallback")]
>> +            (MaintenanceType::UnknownEnumValue(_), _) => Ok(()),
>>           }
>> -        Ok(())
>>       }
>>   }
>