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

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

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