Re: [pve-devel] [PATCH storage] BTRFSPlugin: reuse DirPlugin update/get_volume_attribute

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

On 5/2/22 09:04, Thomas Lamprecht wrote:
> Am 5/2/22 um 08:48 schrieb Dominik Csapak:
>> On 5/2/22 08:36, Thomas Lamprecht wrote:
>>> Am 4/29/22 um 12:00 schrieb Dominik Csapak:
>>>> this allows setting notes+protected for backups on btrfs
>>>>
>>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>>> ---
>>>>    PVE/Storage/BTRFSPlugin.pm | 20 ++++++++++++++++++--
>>>>    1 file changed, 18 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
>>>> index be613f4..dd5f139 100644
>>>> --- a/PVE/Storage/BTRFSPlugin.pm
>>>> +++ b/PVE/Storage/BTRFSPlugin.pm
>>>> @@ -138,9 +138,25 @@ sub status {
>>>>        return PVE::Storage::DirPlugin::status($class, $storeid, $scfg, $cache);
>>>>    }
>>>>    -# TODO: sub get_volume_attribute {}
>>>> +# FIXME remove on the next APIAGE reset.
>>>> +# Deprecated, use get_volume_attribute instead.
>>>> +sub get_volume_notes {
>>>> +    return PVE::Storage::DirPlugin::get_volume_notes(@_);
>>>> +}
>>>> +
>>>> +# FIXME remove on the next APIAGE reset.
>>>> +# Deprecated, use update_volume_attribute instead.
>>>> +sub update_volume_notes {
>>>> +    return PVE::Storage::DirPlugin::update_volume_notes( @_);
>>>> +}
>>>
>>> makes no sense to add these? they are deprecated and unused anyway
>>
>> no actually, the DirPlugin implementation calls
>> $class->get_volume_notes for now, so it would try to call the
>> BtrfsPlugin version of those which inherits from Plugin which dies in those...
>> (CephFs/CIFS/NFS actually do the same as i did here)
> 
> such thing would be good things to include in the commit message ;-)
> 
> Anyhow, as removal of that method is planned anyhow the real fix would be to move
> the implementation in update_volume_notes away from it, be it through inlining the
> small logic or moving it to a new, private, helper.
> 
>>
>> i guess we could do (untested)
>> --8<--
>> shift @_; # discard class
>> PVE::Storage::DirPlugin->update_volume_notes(@_);
>> -->8--
> 
> most often it's way nicer to avoid the (@_) calls in general and do an explicit
> 
> my ($class, $...) = @_;
> return foo($class, $...) # or whatever
> 
> anyway, allows to see the actual "signature" and makes things more explicit.
> 
> 
>>
>> not sure if thats a good idea though
>>
>> we could also factor out the get/update_volume_notes impl in DirPlugin
>> and call it from both paths? then we'd not have to implement
>> the _notes subs here
>>
>>>
>>>>    -# TODO: sub update_volume_attribute {}
>>>> +sub get_volume_attribute {
>>>> +    return PVE::Storage::DirPlugin::get_volume_attribute(@_);
>>>> +}
>>>> +
>>>> +sub update_volume_attribute {
>>>> +    return PVE::Storage::DirPlugin::update_volume_attribute(@_);
>>>> +}
>>>
>>> This is so trivial that I'm wondering if Wolfgang had a reason to not do it for the
>>> original get_volume_notes that was there long before the BTRFS plugin got added..
>>
>> i mean it's possible, but idk how else you'd implement it? notes & protected
>> are only files where we read/write the content or test the existance?
>>
>> we could probably do something btrfs specific, but is it worth that?
> 
> 
> as hinted, Wolfgang will be the one to answer the reason, even if it was just "forgot".
> And yeah, it's IMO worth it to actually understand first why some seemingly trivial feature
> was skipped before just doing something "blindly", seemingly obvious or not.

makes sense, i just noticed because on content listing, the notes will already show up if
set this way because in the 'get_subdir_files' of Storage.pm we directly read the
notes file if it exists