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

[Fiona Ebner <f.ebner@proxmox.com>]

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