[pbs-devel] [PATCH proxmox-backup] tape: mam: actually enforcing the length when writing attributes

[Dominik Csapak]

On 5/24/24 09:50, Thomas Lamprecht wrote:
> Am 23/05/2024 um 12:08 schrieb Dominik Csapak:
>> we have to check against the length of the attribute, not the u16
>> maximum value.
>>
>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>> ---
>>   pbs-tape/src/sg_tape/mam.rs | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/pbs-tape/src/sg_tape/mam.rs b/pbs-tape/src/sg_tape/mam.rs
>> index 4e995d0b..dc5163a5 100644
>> --- a/pbs-tape/src/sg_tape/mam.rs
>> +++ b/pbs-tape/src/sg_tape/mam.rs
>> @@ -169,7 +169,7 @@ pub fn write_mam_attribute<F: AsRawFd>(
>>       attr_data.extend(data);
>>       if !data.is_empty() && data.len() < attribute.len as usize {
>>           attr_data.resize(attr_data.len() - data.len() + attribute.len as usize, 0);
>> -    } else if data.len() > u16::MAX as usize {
>> +    } else if data.len() > attribute.len as usize {
>>           bail!("data to long");
> 
> Can we extend this error with the attribute name, the length we got and
> the defined max length to have some more info for when a user runs into
> this?
> 
> Something like:
> 
> bail!("attribute '{}' length {} is over the maximum allowed length {}", ...);
> 
> Or is this error already extended/concatenated with similar info somewhere
> in the return chain (sorry, just doing a quick review directly in my MUA)


it's logged with:

could not set MAM attribute {id}: {err}

and from the id we can infer the maximum length

but printing the expected/actual len probably make sense