* [PATCH storage] plugins: better document how volname_for_format() validates the name
@ 2026-10-05 11:04 Fiona Ebner
2026-10-07 12:51 ` Elias Huhsovitz
0 siblings, 1 reply; 4+ messages in thread
From: Fiona Ebner @ 2026-10-05 11:04 UTC (permalink / raw)
To: pve-devel
In particular, there is a requirement for get_parsed_format() to
return a format only if it validated the name. Mention it explicitly.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
src/PVE/Storage/Plugin.pm | 5 +++--
src/PVE/Storage/ZFSPoolPlugin.pm | 3 ++-
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 8318a68..e2025f8 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -851,7 +851,7 @@ sub parse_volname {
=head3 get_parsed_format
Return the disk format encoded in the given volume name, or C<undef> if the name does not spell one
-out.
+out. If a format is returned, the name must have been validated.
This is an extension point for plugins whose volume names encode the format differently. ZFS
derives it from the name prefix via C<parse_volname>, while LVM and RBD take it from a known file
@@ -866,7 +866,7 @@ sub get_parsed_format {
return undef if $name !~ m/\.[^.]+$/; # no extension, so no format is spelled out
- return (parse_name_dir($name))[1];
+ return (parse_name_dir($name))[1]; # dies for invalid volume file names
}
sub is_valid_format {
@@ -904,6 +904,7 @@ sub volname_for_format {
my $parsed_volname_fmt = $class->get_parsed_format($name);
+ # Note that get_parsed_format() validates the name if it has a format extension.
return $name if defined($parsed_volname_fmt) && $parsed_volname_fmt eq $fmt;
my $suggestion = $class->volname_with_format($name, $fmt);
diff --git a/src/PVE/Storage/ZFSPoolPlugin.pm b/src/PVE/Storage/ZFSPoolPlugin.pm
index 61c49d9..e7cd1de 100644
--- a/src/PVE/Storage/ZFSPoolPlugin.pm
+++ b/src/PVE/Storage/ZFSPoolPlugin.pm
@@ -159,7 +159,7 @@ sub parse_volname {
sub get_parsed_format {
my ($class, $name) = @_;
- return ($class->parse_volname($name))[6];
+ return ($class->parse_volname($name))[6]; # dies for invalid volume names
}
# ZFS volume names always encode their format in the name prefix (vm- for raw
@@ -170,6 +170,7 @@ sub volname_for_format {
die "unsupported format '$fmt'\n" if !($class->is_valid_format($fmt));
+ # Note that get_parsed_format() validates the name.
my $name_fmt = $class->get_parsed_format($name);
return $name if $name_fmt eq $fmt;
--
2.47.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH storage] plugins: better document how volname_for_format() validates the name
2026-10-05 11:04 [PATCH storage] plugins: better document how volname_for_format() validates the name Fiona Ebner
@ 2026-10-07 12:51 ` Elias Huhsovitz
2026-10-07 13:12 ` Fiona Ebner
0 siblings, 1 reply; 4+ messages in thread
From: Elias Huhsovitz @ 2026-10-07 12:51 UTC (permalink / raw)
To: Fiona Ebner, pve-devel
I think the comments here are fine, but I would prefer this should be
documented in the POD for the respective subroutines.
Both parse_name_dir & (espeically) parse_volname would also benefit
from a proper POD.
See small comments inlinde.
On Mon Oct 5, 2026 at 1:04 PM CEST, Fiona Ebner wrote:
> In particular, there is a requirement for get_parsed_format() to
> return a format only if it validated the name. Mention it explicitly.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> src/PVE/Storage/Plugin.pm | 5 +++--
> src/PVE/Storage/ZFSPoolPlugin.pm | 3 ++-
> 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 8318a68..e2025f8 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -851,7 +851,7 @@ sub parse_volname {
> =head3 get_parsed_format
>
> Return the disk format encoded in the given volume name, or C<undef> if the name does not spell one
> -out.
> +out. If a format is returned, the name must have been validated.
>
> This is an extension point for plugins whose volume names encode the format differently. ZFS
> derives it from the name prefix via C<parse_volname>, while LVM and RBD take it from a known file
> @@ -866,7 +866,7 @@ sub get_parsed_format {
>
> return undef if $name !~ m/\.[^.]+$/; # no extension, so no format is spelled out
>
> - return (parse_name_dir($name))[1];
> + return (parse_name_dir($name))[1]; # dies for invalid volume file names
We could add this information inside the POD of the function.
Similarly to how javadoc has the @throws annonation.
> }
>
> sub is_valid_format {
> @@ -904,6 +904,7 @@ sub volname_for_format {
>
> my $parsed_volname_fmt = $class->get_parsed_format($name);
>
> + # Note that get_parsed_format() validates the name if it has a format extension.
> return $name if defined($parsed_volname_fmt) && $parsed_volname_fmt eq $fmt;
>
> my $suggestion = $class->volname_with_format($name, $fmt);
> diff --git a/src/PVE/Storage/ZFSPoolPlugin.pm b/src/PVE/Storage/ZFSPoolPlugin.pm
> index 61c49d9..e7cd1de 100644
> --- a/src/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/src/PVE/Storage/ZFSPoolPlugin.pm
> @@ -159,7 +159,7 @@ sub parse_volname {
> sub get_parsed_format {
> my ($class, $name) = @_;
>
> - return ($class->parse_volname($name))[6];
> + return ($class->parse_volname($name))[6]; # dies for invalid volume names
Same here, I belive this belongs in the POD, as part of the "contract".
> }
>
> # ZFS volume names always encode their format in the name prefix (vm- for raw
> @@ -170,6 +170,7 @@ sub volname_for_format {
>
> die "unsupported format '$fmt'\n" if !($class->is_valid_format($fmt));
>
> + # Note that get_parsed_format() validates the name.
> my $name_fmt = $class->get_parsed_format($name);
> return $name if $name_fmt eq $fmt;
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH storage] plugins: better document how volname_for_format() validates the name
2026-10-07 12:51 ` Elias Huhsovitz
@ 2026-10-07 13:12 ` Fiona Ebner
2026-10-08 10:29 ` Elias Huhsovitz
0 siblings, 1 reply; 4+ messages in thread
From: Fiona Ebner @ 2026-10-07 13:12 UTC (permalink / raw)
To: Elias Huhsovitz, pve-devel
Am 07.10.26 um 2:51 PM schrieb Elias Huhsovitz:
> I think the comments here are fine, but I would prefer this should be
> documented in the POD for the respective subroutines.
>
> Both parse_name_dir & (espeically) parse_volname would also benefit
> from a proper POD.
Yes, but that is out-of-scope for this patch. Max has been working on
documenting the storage plugin interface properly. This patch is for
better documenting how the volname_for_format() implementations achieve
validation, and even if parse_{name_dir,volname} had proper POD, the
added comments inline are better suited for making it explicit/readable
how validation is achieved here IMHO.
>
> See small comments inlinde.
>
> On Mon Oct 5, 2026 at 1:04 PM CEST, Fiona Ebner wrote:
>> In particular, there is a requirement for get_parsed_format() to
>> return a format only if it validated the name. Mention it explicitly.
>>
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>> src/PVE/Storage/Plugin.pm | 5 +++--
>> src/PVE/Storage/ZFSPoolPlugin.pm | 3 ++-
>> 2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
>> index 8318a68..e2025f8 100644
>> --- a/src/PVE/Storage/Plugin.pm
>> +++ b/src/PVE/Storage/Plugin.pm
>> @@ -851,7 +851,7 @@ sub parse_volname {
>> =head3 get_parsed_format
>>
>> Return the disk format encoded in the given volume name, or C<undef> if the name does not spell one
>> -out.
>> +out. If a format is returned, the name must have been validated.
>>
>> This is an extension point for plugins whose volume names encode the format differently. ZFS
>> derives it from the name prefix via C<parse_volname>, while LVM and RBD take it from a known file
>> @@ -866,7 +866,7 @@ sub get_parsed_format {
>>
>> return undef if $name !~ m/\.[^.]+$/; # no extension, so no format is spelled out
>>
>> - return (parse_name_dir($name))[1];
>> + return (parse_name_dir($name))[1]; # dies for invalid volume file names
>
> We could add this information inside the POD of the function.
>
> Similarly to how javadoc has the @throws annonation.
>
>> }
>>
>> sub is_valid_format {
>> @@ -904,6 +904,7 @@ sub volname_for_format {
>>
>> my $parsed_volname_fmt = $class->get_parsed_format($name);
>>
>> + # Note that get_parsed_format() validates the name if it has a format extension.
>> return $name if defined($parsed_volname_fmt) && $parsed_volname_fmt eq $fmt;
>>
>> my $suggestion = $class->volname_with_format($name, $fmt);
>> diff --git a/src/PVE/Storage/ZFSPoolPlugin.pm b/src/PVE/Storage/ZFSPoolPlugin.pm
>> index 61c49d9..e7cd1de 100644
>> --- a/src/PVE/Storage/ZFSPoolPlugin.pm
>> +++ b/src/PVE/Storage/ZFSPoolPlugin.pm
>> @@ -159,7 +159,7 @@ sub parse_volname {
>> sub get_parsed_format {
>> my ($class, $name) = @_;
>>
>> - return ($class->parse_volname($name))[6];
>> + return ($class->parse_volname($name))[6]; # dies for invalid volume names
>
> Same here, I belive this belongs in the POD, as part of the "contract".
>
>> }
>>
>> # ZFS volume names always encode their format in the name prefix (vm- for raw
>> @@ -170,6 +170,7 @@ sub volname_for_format {
>>
>> die "unsupported format '$fmt'\n" if !($class->is_valid_format($fmt));
>>
>> + # Note that get_parsed_format() validates the name.
>> my $name_fmt = $class->get_parsed_format($name);
>> return $name if $name_fmt eq $fmt;
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH storage] plugins: better document how volname_for_format() validates the name
2026-10-07 13:12 ` Fiona Ebner
@ 2026-10-08 10:29 ` Elias Huhsovitz
0 siblings, 0 replies; 4+ messages in thread
From: Elias Huhsovitz @ 2026-10-08 10:29 UTC (permalink / raw)
To: Fiona Ebner, pve-devel
On Wed Oct 7, 2026 at 3:12 PM CEST, Fiona Ebner wrote:
> Am 07.10.26 um 2:51 PM schrieb Elias Huhsovitz:
>> I think the comments here are fine, but I would prefer this should be
>> documented in the POD for the respective subroutines.
>>
>> Both parse_name_dir & (espeically) parse_volname would also benefit
>> from a proper POD.
>
> Yes, but that is out-of-scope for this patch. Max has been working on
> documenting the storage plugin interface properly.
Ok, then I do not mind the comments. I was just afraid that the
documentation might end at the comments ;D.
LGTM!
> This patch is for
> better documenting how the volname_for_format() implementations achieve
> validation, and even if parse_{name_dir,volname} had proper POD, the
> added comments inline are better suited for making it explicit/readable
> how validation is achieved here IMHO.
>
>>
>> See small comments inlinde.
>>
>> On Mon Oct 5, 2026 at 1:04 PM CEST, Fiona Ebner wrote:
>>> In particular, there is a requirement for get_parsed_format() to
>>> return a format only if it validated the name. Mention it explicitly.
>>>
>>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>>> ---
>>> src/PVE/Storage/Plugin.pm | 5 +++--
>>> src/PVE/Storage/ZFSPoolPlugin.pm | 3 ++-
>>> 2 files changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
>>> index 8318a68..e2025f8 100644
>>> --- a/src/PVE/Storage/Plugin.pm
>>> +++ b/src/PVE/Storage/Plugin.pm
>>> @@ -851,7 +851,7 @@ sub parse_volname {
>>> =head3 get_parsed_format
>>>
>>> Return the disk format encoded in the given volume name, or C<undef> if the name does not spell one
>>> -out.
>>> +out. If a format is returned, the name must have been validated.
>>>
>>> This is an extension point for plugins whose volume names encode the format differently. ZFS
>>> derives it from the name prefix via C<parse_volname>, while LVM and RBD take it from a known file
>>> @@ -866,7 +866,7 @@ sub get_parsed_format {
>>>
>>> return undef if $name !~ m/\.[^.]+$/; # no extension, so no format is spelled out
>>>
>>> - return (parse_name_dir($name))[1];
>>> + return (parse_name_dir($name))[1]; # dies for invalid volume file names
>>
>> We could add this information inside the POD of the function.
>>
>> Similarly to how javadoc has the @throws annonation.
>>
>>> }
>>>
>>> sub is_valid_format {
>>> @@ -904,6 +904,7 @@ sub volname_for_format {
>>>
>>> my $parsed_volname_fmt = $class->get_parsed_format($name);
>>>
>>> + # Note that get_parsed_format() validates the name if it has a format extension.
>>> return $name if defined($parsed_volname_fmt) && $parsed_volname_fmt eq $fmt;
>>>
>>> my $suggestion = $class->volname_with_format($name, $fmt);
>>> diff --git a/src/PVE/Storage/ZFSPoolPlugin.pm b/src/PVE/Storage/ZFSPoolPlugin.pm
>>> index 61c49d9..e7cd1de 100644
>>> --- a/src/PVE/Storage/ZFSPoolPlugin.pm
>>> +++ b/src/PVE/Storage/ZFSPoolPlugin.pm
>>> @@ -159,7 +159,7 @@ sub parse_volname {
>>> sub get_parsed_format {
>>> my ($class, $name) = @_;
>>>
>>> - return ($class->parse_volname($name))[6];
>>> + return ($class->parse_volname($name))[6]; # dies for invalid volume names
>>
>> Same here, I belive this belongs in the POD, as part of the "contract".
>>
>>> }
>>>
>>> # ZFS volume names always encode their format in the name prefix (vm- for raw
>>> @@ -170,6 +170,7 @@ sub volname_for_format {
>>>
>>> die "unsupported format '$fmt'\n" if !($class->is_valid_format($fmt));
>>>
>>> + # Note that get_parsed_format() validates the name.
>>> my $name_fmt = $class->get_parsed_format($name);
>>> return $name if $name_fmt eq $fmt;
>>>
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-10-08 10:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-05 11:04 [PATCH storage] plugins: better document how volname_for_format() validates the name Fiona Ebner
2026-10-07 12:51 ` Elias Huhsovitz
2026-10-07 13:12 ` Fiona Ebner
2026-10-08 10:29 ` Elias Huhsovitz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox