Re: [PATCH storage] plugins: better document how volname_for_format() validates the name

["Elias Huhsovitz" <e.huhsovitz@proxmox.com>]

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;
>>>  
>>