[pve-devel] [PATCH storage v2 1/2] DirPlugin: update_volume_attribute: don't use update_volume_notes

[pve-devel] [PATCH storage v2 1/2] DirPlugin: update_volume_attribute: don't use update_volume_notes

[Dominik Csapak]

by refactoring it into a helper and using that. With this, we can omit the
'update_volume_notes' in subclasses, when they did not support it before

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/Storage/DirPlugin.pm | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
index 53eb642..8715a9d 100644
--- a/PVE/Storage/DirPlugin.pm
+++ b/PVE/Storage/DirPlugin.pm
@@ -94,9 +94,8 @@ sub parse_is_mountpoint {
     return $is_mp; # contains a path
 }
 
-# FIXME remove on the next APIAGE reset.
-# Deprecated, use get_volume_attribute instead.
-sub get_volume_notes {
+# FIXME move into 'get_volume_attribute' when removing 'get_volume_notes'
+my $get_volume_notes_impl = sub {
     my ($class, $scfg, $storeid, $volname, $timeout) = @_;
 
     my ($vtype) = $class->parse_volname($volname);
@@ -111,11 +110,17 @@ sub get_volume_notes {
     }
 
     return '';
-}
+};
 
 # FIXME remove on the next APIAGE reset.
-# Deprecated, use update_volume_attribute instead.
-sub update_volume_notes {
+# Deprecated, use get_volume_attribute instead.
+sub get_volume_notes {
+    my ($class, $scfg, $storeid, $volname, $timeout) = @_;
+    return $get_volume_notes_impl->($class, $scfg, $storeid, $volname, $timeout);
+}
+
+# FIXME move into 'update_volume_attribute' when removing 'update_volume_notes'
+my $update_volume_notes_impl = sub {
     my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
 
     my ($vtype) = $class->parse_volname($volname);
@@ -131,13 +136,20 @@ sub update_volume_notes {
 	unlink $path or $! == ENOENT or die "could not delete notes - $!\n";
     }
     return;
+};
+
+# FIXME remove on the next APIAGE reset.
+# Deprecated, use update_volume_attribute instead.
+sub update_volume_notes {
+    my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
+    return $update_volume_notes_impl->($class, $scfg, $storeid, $volname, $notes, $timeout);
 }
 
 sub get_volume_attribute {
     my ($class, $scfg, $storeid, $volname, $attribute) = @_;
 
     if ($attribute eq 'notes') {
-	return $class->get_volume_notes($scfg, $storeid, $volname);
+	return $get_volume_notes_impl->($class, $scfg, $storeid, $volname);
     }
 
     my ($vtype) = $class->parse_volname($volname);
@@ -155,7 +167,7 @@ sub update_volume_attribute {
     my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
 
     if ($attribute eq 'notes') {
-	return $class->update_volume_notes($scfg, $storeid, $volname, $value);
+	return $update_volume_notes_impl->($class, $scfg, $storeid, $volname, $value);
     }
 
     my ($vtype) = $class->parse_volname($volname);
-- 
2.30.2

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

[Dominik Csapak]

this allows setting notes+protected for backups on btrfs

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/Storage/BTRFSPlugin.pm | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
index be613f4..55f3c05 100644
--- a/PVE/Storage/BTRFSPlugin.pm
+++ b/PVE/Storage/BTRFSPlugin.pm
@@ -138,9 +138,16 @@ sub status {
     return PVE::Storage::DirPlugin::status($class, $storeid, $scfg, $cache);
 }
 
-# TODO: sub get_volume_attribute {}
+sub get_volume_attribute {
+    my ($class, $scfg, $storeid, $volname, $attribute) = @_;
+    return PVE::Storage::DirPlugin::get_volume_attribute($class, $scfg, $storeid, $volname, $attribute);
+}
 
-# TODO: sub update_volume_attribute {}
+sub update_volume_attribute {
+    my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
+    return PVE::Storage::DirPlugin::update_volume_attribute($class, $scfg, $storeid, $volname,
+	$attribute, $value);
+}
 
 # croak would not include the caller from within this module
 sub __error {
-- 
2.30.2

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

[Thomas Lamprecht]

Am 27/05/2022 um 14:31 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 | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)

looks OK, but the whole situation with that and often also btrfs is sometimes
a bit weird in subtle ways, so a quick look from you wolfgang would be appreciated.

Also, one style nit inline.

> 
> diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
> index be613f4..55f3c05 100644
> --- a/PVE/Storage/BTRFSPlugin.pm
> +++ b/PVE/Storage/BTRFSPlugin.pm
> @@ -138,9 +138,16 @@ sub status {
>      return PVE::Storage::DirPlugin::status($class, $storeid, $scfg, $cache);
>  }
>  
> -# TODO: sub get_volume_attribute {}
> +sub get_volume_attribute {
> +    my ($class, $scfg, $storeid, $volname, $attribute) = @_;
> +    return PVE::Storage::DirPlugin::get_volume_attribute($class, $scfg, $storeid, $volname, $attribute);
> +}
>  
> -# TODO: sub update_volume_attribute {}
> +sub update_volume_attribute {
> +    my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
> +    return PVE::Storage::DirPlugin::update_volume_attribute($class, $scfg, $storeid, $volname,
> +	$attribute, $value);

I'd actually prefer the rustfmt style for wrapping simple lists of parameters, in that
case the following should make it happy, well theoretically as it would choke on perl,
as lots others do:

return PVE::Storage::DirPlugin::update_volume_attribute(
    $class, $scfg, $storeid, $volname, $attribute, $value);

And if that'd still require to much width it'd go one param per line.
I'll see that I update the style guide with that.

> +}
>  
>  # croak would not include the caller from within this module
>  sub __error {

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

[Wolfgang Bumiller]

On Thu, Jun 02, 2022 at 09:14:59AM +0200, Thomas Lamprecht wrote:
> Am 27/05/2022 um 14:31 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 | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> looks OK, but the whole situation with that and often also btrfs is sometimes
> a bit weird in subtle ways, so a quick look from you wolfgang would be appreciated.

I'm a bit confused about the `get/update_volume_notes` subs.
You said they are deprecated and unused. However, the DirPlugin does
call it when `$attribute eq "notes"` in `get/update_volume_attribute`.

Since BTRFSPlugin uses Plugin as a base and not DirPlugin and we pass
$class (== BTRFSPlugin) to it, setting notes should not work with this patch?

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

[Dominik Csapak]

On 6/2/22 10:03, Wolfgang Bumiller wrote:
> On Thu, Jun 02, 2022 at 09:14:59AM +0200, Thomas Lamprecht wrote:
>> Am 27/05/2022 um 14:31 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 | 11 +++++++++--
>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> looks OK, but the whole situation with that and often also btrfs is sometimes
>> a bit weird in subtle ways, so a quick look from you wolfgang would be appreciated.
> 
> I'm a bit confused about the `get/update_volume_notes` subs.
> You said they are deprecated and unused. However, the DirPlugin does
> call it when `$attribute eq "notes"` in `get/update_volume_attribute`.
> 
> Since BTRFSPlugin uses Plugin as a base and not DirPlugin and we pass
> $class (== BTRFSPlugin) to it, setting notes should not work with this patch?


in the first patch of the series i refactor that, so *_volume_attribute
does not call *_volume_notes anymore

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

[Wolfgang Bumiller]

On Thu, Jun 02, 2022 at 10:03:32AM +0200, Wolfgang Bumiller wrote:
> On Thu, Jun 02, 2022 at 09:14:59AM +0200, Thomas Lamprecht wrote:
> > Am 27/05/2022 um 14:31 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 | 11 +++++++++--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > looks OK, but the whole situation with that and often also btrfs is sometimes
> > a bit weird in subtle ways, so a quick look from you wolfgang would be appreciated.
> 
> I'm a bit confused about the `get/update_volume_notes` subs.
> You said they are deprecated and unused. However, the DirPlugin does
> call it when `$attribute eq "notes"` in `get/update_volume_attribute`.
> 
> Since BTRFSPlugin uses Plugin as a base and not DirPlugin and we pass
> $class (== BTRFSPlugin) to it, setting notes should not work with this patch?

Nevermind, I didn't read the 1st patch properly, those aren't called via
$class anymore.

Acked-by: Wolfgang Bumiller <w.bumiller@proxmox.com>