[PATCH storage 2/3] plugin: generalize volname_for_format across storage plugins

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

Move `volname_for_format()` from `LVMPlugin` into `Plugin` to make it
reusable for all storage backends. This function reconciles the
requested format with the volume name and ensures they match.

Introduce `get_parsed_format()` as an extension point. Subclasses whose
volume names encode the format differently (e.g., LVM, ZFS) override
this method to derive the format via `parse_volname()`.

Add `FORMAT_EXTENSION` constants to affected plugins, enabling the
generation of useful format mismatch suggestions.

Call `volname_for_format()` in `alloc_image()` and `rename_volume()`
for the `Plugin`, `BTRFS`, `LVM`, `LvmThin`, `RBD`, `ZFS`, and `ZFSPool`
backends.

In `ZFSPoolPlugin`, override `volname_for_format()` and
`volname_with_format()` to handle its unique prefix-based naming scheme
(e.g., `vm-` vs `subvol-`).

Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
---
 src/PVE/Storage/BTRFSPlugin.pm   |  6 +-
 src/PVE/Storage/LVMPlugin.pm     | 37 ++++++------
 src/PVE/Storage/LvmThinPlugin.pm | 12 ++++
 src/PVE/Storage/Plugin.pm        | 97 ++++++++++++++++++++++++++++++--
 src/PVE/Storage/RBDPlugin.pm     | 14 +++++
 src/PVE/Storage/ZFSPlugin.pm     |  2 +
 src/PVE/Storage/ZFSPoolPlugin.pm | 37 ++++++++++++
 7 files changed, 176 insertions(+), 29 deletions(-)

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index fb47aa0..fa28592 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -333,10 +333,7 @@ sub alloc_image {
 
     $name = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt, 1) if !$name;
 
-    my (undef, $tmpfmt) = PVE::Storage::Plugin::parse_name_dir($name);
-
-    die "illegal name '$name' - wrong extension for format ('$tmpfmt != '$fmt')\n"
-        if $tmpfmt ne $fmt;
+    $name = $class->volname_for_format($name, $fmt, 0);
 
     # End copy from Plugin.pm
 
@@ -974,6 +971,7 @@ sub rename_volume {
 
     $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid, $format, 1)
         if !$target_volname;
+    $target_volname = $class->volname_for_format($target_volname, $format, 0);
     $target_volname = "$target_vmid/$target_volname";
 
     my $basedir = $class->get_subdir($scfg, 'images');
diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 0f2ef66..6ed613d 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -16,6 +16,16 @@ use PVE::Storage::Plugin;
 
 use base qw(PVE::Storage::Plugin);
 
+use constant FORMAT_EXTENSION => {
+    raw => '',
+    qcow2 => 'qcow2',
+};
+
+use constant EXTENSION_FORMAT => {
+    '' => 'raw',
+    'qcow2' => 'qcow2',
+};
+
 # lvm helper functions
 
 my $ignore_no_medium_warnings = sub {
@@ -738,27 +748,10 @@ my sub alloc_lvm_image {
 
 }
 
-# Only 'qcow2' names carry an extension, like the ones find_free_diskname() generates, so add it
-# for callers passing a fixed name, like cloud-init drives. A name that already spells out another
-# format stays an error, it states an intent that the requested format contradicts.
-my sub volname_for_format {
-    my ($class, $name, $fmt) = @_;
-
-    return $name if $fmt ne 'raw' && $fmt ne 'qcow2'; # alloc_lvm_image() reports unsupported ones
+sub get_parsed_format {
+    my ($class, $name) = @_;
 
-    my $name_fmt = ($class->parse_volname($name))[6];
-    return $name if $name_fmt eq $fmt;
-
-    if ($fmt eq 'raw') {
-        my $suggested_name = $name =~ s/\.\Q$name_fmt\E$//r;
-        die "volume name '$name' does not match requested format '$fmt'"
-            . " (did you mean '$suggested_name'?)\n";
-    }
-
-    my $adapted_name = "$name.$fmt";
-    warn "volume name '$name' is missing the '.$fmt' extension - allocating '$adapted_name'\n";
-
-    return $adapted_name;
+    return ($class->parse_volname($name))[6];
 }
 
 sub alloc_image {
@@ -767,7 +760,7 @@ sub alloc_image {
     $name = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt)
         if !$name;
 
-    $name = volname_for_format($class, $name, $fmt);
+    $name = $class->volname_for_format($name, $fmt, 0);
 
     alloc_lvm_image($class, $storeid, $scfg, $vmid, $fmt, $name, $size);
 
@@ -1542,6 +1535,8 @@ sub rename_volume {
     $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid, $format)
         if !$target_volname;
 
+    $target_volname = $class->volname_for_format($target_volname, $format, 0);
+
     my $vg = $scfg->{vgname};
     my $lvs = lvm_list_volumes($vg);
     die "target volume '${target_volname}' already exists\n"
diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
index d44e04d..0c25973 100644
--- a/src/PVE/Storage/LvmThinPlugin.pm
+++ b/src/PVE/Storage/LvmThinPlugin.pm
@@ -25,6 +25,10 @@ use PVE::Storage::LVMPlugin;
 
 use base qw(PVE::Storage::LVMPlugin);
 
+use constant FORMAT_EXTENSION => {
+    raw => '',
+};
+
 sub type {
     return 'lvmthin';
 }
@@ -100,6 +104,12 @@ my $set_lv_autoactivation = sub {
     warn "could not set autoactivation: $@" if $@;
 };
 
+sub get_parsed_format {
+    my ($class, $name) = @_;
+
+    return ($class->parse_volname($name))[6];
+}
+
 sub alloc_image {
     my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
 
@@ -117,6 +127,8 @@ sub alloc_image {
     $name = $class->find_free_diskname($storeid, $scfg, $vmid)
         if !$name;
 
+    $name = $class->volname_for_format($name, $fmt, 0);
+
     my $cmd = [
         '/sbin/lvcreate',
         '-aly',
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index c5046a4..d596397 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -27,6 +27,13 @@ use constant COMPRESSOR_RE => join('|', KNOWN_COMPRESSION_FORMATS);
 use constant LOG_EXT => ".log";
 use constant NOTES_EXT => ".notes";
 
+use constant FORMAT_EXTENSION => {
+    raw => 'raw',
+    qcow2 => 'qcow2',
+    vmdk => 'vmdk',
+    subvol => 'subvol',
+};
+
 our @COMMON_TAR_FLAGS = qw(
     --one-file-system
     -p --sparse --numeric-owner --acls
@@ -838,6 +845,89 @@ sub parse_volname {
     die "unable to parse directory volume name '$volname'\n";
 }
 
+=item get_parsed_format($class, $name)
+
+Return the disk format encoded in the given volume name.
+
+This is an extension point. Subclasses whose volume names encode the format
+differently override this method. LVM and ZFS, for example, derive the format
+via C<parse_volname> .
+
+returns: the format indicated by the name.
+
+=cut
+
+sub get_parsed_format {
+    my ($class, $name) = @_;
+
+    return (parse_name_dir($name))[1];
+}
+
+sub is_valid_format {
+    my ($class, $fmt) = @_;
+
+    return defined($class->FORMAT_EXTENSION->{$fmt});
+}
+
+=item volname_for_format($class, $name, $fmt, $strict)
+
+Reconcile the requested format with the volume name and return the name to
+allocate.
+
+Dies immediately if C<$fmt> is not a valid format for this plugin.
+
+If the name already encodes the requested format, returns the name unchanged.
+
+If C<$strict> is true, any format mismatch dies with a suggestion for the
+corrected name.
+
+If C<$strict> is false (lax mode), a name without a file extension is adapted
+to the requested format and a warning is emitted. A name that already carries
+a file extension dies with a suggestion, because the extension indicates an
+explicit format choice that contradicts the request.
+
+returns: the name to allocate
+
+=cut
+
+sub volname_for_format {
+    my ($class, $name, $fmt, $strict) = @_;
+
+    die "unsupported format '$fmt'\n" if !($class->is_valid_format($fmt));
+
+    my $parsed_volname_fmt = $class->get_parsed_format($name);
+
+    return $name if $parsed_volname_fmt eq $fmt;
+
+    my $suggestion = $class->volname_with_format($name, $fmt);
+
+    if ($strict || $name =~ /\.[^.]+$/) {
+        die "illegal name $name - volume name does not match requested format "
+            . "'$fmt' (did you mean '$suggestion'?)\n";
+    }
+
+    warn "volume name '$name' is missing the '.$fmt' extension - allocating '$suggestion'\n";
+
+    return $suggestion;
+}
+
+sub get_format_extension {
+    my ($class, $fmt) = @_;
+
+    return $class->FORMAT_EXTENSION->{$fmt};
+}
+
+sub volname_with_format {
+    my ($class, $name, $fmt) = @_;
+
+    my $correct_fmt_ext = $class->get_format_extension($fmt);
+
+    $name =~ s/\.[^.]+$//;
+
+    return $name if !$correct_fmt_ext;
+    return "$name.$correct_fmt_ext";
+}
+
 my $vtype_subdirs = {
     images => 'images',
     rootdir => 'private',
@@ -1058,10 +1148,7 @@ sub alloc_image {
 
     $name = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt, 1) if !$name;
 
-    my (undef, $tmpfmt) = parse_name_dir($name);
-
-    die "illegal name '$name' - wrong extension for format ('$tmpfmt != '$fmt')\n"
-        if $tmpfmt ne $fmt;
+    $name = $class->volname_for_format($name, $fmt, 0);
 
     my $path = "$imagedir/$name";
 
@@ -2391,6 +2478,8 @@ sub rename_volume {
     $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid, $format, 1)
         if !$target_volname;
 
+    $target_volname = $class->volname_for_format($target_volname, $format, 0);
+
     my $basedir = $class->get_subdir($scfg, 'images');
 
     mkpath "${basedir}/${target_vmid}";
diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index 8ebb999..95b65d6 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -22,6 +22,10 @@ use PVE::Storage::Common;
 
 use base qw(PVE::Storage::Plugin);
 
+use constant FORMAT_EXTENSION => {
+    raw => '',
+};
+
 my $get_parent_image_name = sub {
     my ($parent) = @_;
     return undef if !$parent;
@@ -522,6 +526,12 @@ sub parse_volname {
     die "unable to parse rbd volume name '$volname'\n";
 }
 
+sub get_parsed_format {
+    my ($class, $name) = @_;
+
+    return ($class->parse_volname($name))[6];
+}
+
 sub path {
     my ($class, $scfg, $volname, $storeid, $snapname) = @_;
 
@@ -715,6 +725,8 @@ sub alloc_image {
 
     $name = $class->find_free_diskname($storeid, $scfg, $vmid) if !$name;
 
+    $name = $class->volname_for_format($name, $fmt, 0);
+
     my @options = (
         '--image-format', 2, '--size', int(($size + 1023) / 1024),
     );
@@ -1087,6 +1099,8 @@ sub rename_volume {
     $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid, $format)
         if !$target_volname;
 
+    $target_volname = $class->volname_for_format($target_volname, $format, 0);
+
     die "target volume '${target_volname}' already exists\n"
         if rbd_volume_exists($scfg, $storeid, $target_volname);
 
diff --git a/src/PVE/Storage/ZFSPlugin.pm b/src/PVE/Storage/ZFSPlugin.pm
index 74e0a08..bfae513 100644
--- a/src/PVE/Storage/ZFSPlugin.pm
+++ b/src/PVE/Storage/ZFSPlugin.pm
@@ -366,6 +366,8 @@ sub alloc_image {
 
     $volname = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt) if !$volname;
 
+    $volname = $class->volname_for_format($volname, $fmt, 0);
+
     $class->zfs_create_zvol($scfg, $volname, $size);
 
     my $guid = $class->zfs_create_lu($scfg, $volname);
diff --git a/src/PVE/Storage/ZFSPoolPlugin.pm b/src/PVE/Storage/ZFSPoolPlugin.pm
index 55f0bbc..4fecebb 100644
--- a/src/PVE/Storage/ZFSPoolPlugin.pm
+++ b/src/PVE/Storage/ZFSPoolPlugin.pm
@@ -150,6 +150,37 @@ sub parse_volname {
     die "unable to parse zfs volume name '$volname'\n";
 }
 
+sub get_parsed_format {
+    my ($class, $name) = @_;
+
+    return ($class->parse_volname($name))[6];
+}
+
+# ZFS volume names always encode their format in the name prefix (vm- for raw
+# zvols, subvol- for subvolumes). Any mismatch therefore spells out a
+# contradicting format and is an error, regardless of $strict.
+sub volname_for_format {
+    my ($class, $name, $fmt, $strict) = @_;
+
+    my $name_fmt = $class->get_parsed_format($name);
+    return $name if $name_fmt eq $fmt;
+
+    my $suggestion = $class->volname_with_format($name, $name_fmt, $fmt);
+
+    die "illegal name $name - volume name does not match requested format "
+        . "'$fmt' (did you mean '$suggestion'?)\n";
+}
+
+sub volname_with_format {
+    my ($class, $name, $name_fmt, $fmt) = @_;
+
+    if ($fmt eq 'subvol') {
+        return $name =~ s/^(?:vm|base|basevol)-/subvol-/r;
+    }
+
+    return $name =~ s/^(?:subvol|basevol|base)-/vm-/r;
+}
+
 # virtual zfs methods (subclass can overwrite them)
 
 sub on_add_hook {
@@ -273,6 +304,8 @@ sub alloc_image {
         $volname = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt)
             if !$volname;
 
+        $volname = $class->volname_for_format($volname, $fmt, 0);
+
         $class->zfs_create_zvol($scfg, $volname, $size);
         $class->zfs_wait_for_zvol_link($scfg, $volname);
 
@@ -283,6 +316,8 @@ sub alloc_image {
         $volname = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt)
             if !$volname;
 
+        $volname = $class->volname_for_format($volname, $fmt, 0);
+
         die "illegal name '$volname' - should be 'subvol-$vmid-*'\n"
             if $volname !~ m/^subvol-$vmid-/;
 
@@ -960,6 +995,8 @@ sub rename_volume {
     $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid, $format)
         if !$target_volname;
 
+    $target_volname = $class->volname_for_format($target_volname, $format, 0);
+
     my $pool = $scfg->{pool};
     my $source_zfspath = "${pool}/${source_image}";
     my $target_zfspath = "${pool}/${target_volname}";
-- 
2.47.3