On 8/2/21 2:56 PM, Fabian Ebner wrote:
Am 19.07.21 um 16:52 schrieb Aaron Lauterer:
We do not expose the parameter 'add_fmt_suffix' used by the internal
implemantion of 'find_free_diskname'. This is something only the plugins
themselves know but cannot be determined easily and reliably from an
outside caller.
This is why the new 'wants_fmt_suffix' method has been introduced. For
most plugins the return value is very clear. For the default
implementation in Plugin.pm we add another check to be on the safe side
and only return true if the '$scfg->{path}' option is present.
It indicates that the volume in question is an actual file which will
need the suffix.
Signed-off-by: Aaron Lauterer <a.laute...@proxmox.com>
--- > rfc -> v1:
dropped $add_fmt_suffix parameter and added the "wants_fmt_suffix"
helper method in each plugin.
PVE/Storage.pm | 11 +++++++++++
PVE/Storage/LVMPlugin.pm | 5 +++++
PVE/Storage/Plugin.pm | 7 +++++++
PVE/Storage/RBDPlugin.pm | 5 +++++
PVE/Storage/ZFSPoolPlugin.pm | 5 +++++
5 files changed, 33 insertions(+)
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index c04b5a2..afeb2e3 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -203,6 +203,17 @@ sub storage_can_replicate {
return $plugin->storage_can_replicate($scfg, $storeid, $format);
}
+sub find_free_diskname {
+ my ($cfg, $storeid, $vmid, $fmt) = @_;
+
+ my $scfg = storage_config($cfg, $storeid);
+ my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
+
+ my $add_fmt_suffix = $plugin->wants_fmt_suffix($scfg);
+
+ return $plugin->find_free_diskname($storeid, $scfg, $vmid, $fmt,
$add_fmt_suffix);
Maybe this should rather be find_free_volname and always return the full volname (i.e.
"<VMID>/vm-<VMID>-disk-<N>.<FMT>" for dir-based storages) so that it can be
parsed with parse_volname later?
That would mean something like wants_vmid_prefix() is needed, but I'd rather
extend (and rename) the wants_fmt_suffix method instead.
The rationale is: the value returned here is used for the $target_volname
parameter in the next patch, and in case it's a dir-based storage, passing in
an actual volname like '123/vm-123-disk-0.raw' currently fails (because it
actually expects only the disk name).
Adding a new "find_free_volname" is probably the cleanest approach. For the default directory implementation we can return it in the
full "<VMID>/vm-<VMID>-disk-<N>.<FMT>" format. All other storage plugins would need their own implementation
though and for most it will be basically a thin wrapper around "find_free_diskname". Alternatively it might work to check against
$scfg->{path} to decide if we should return just what we get from "find_free_diskname" (LVM, RBD, ZFS,...) or add the VMID prefix and
FMT suffix (files/directory)?
What do you have in mind regarding "wants_fmt_suffix" exactly? If I am not
mistaken, we usually need both (FMT suffix and VMID prefix) as it is usually needed for
directory based storages. All other storages (AFAICT) will not have either.
+}
+
sub storage_ids {
my ($cfg) = @_;
diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
index 139d391..3e5b6c8 100644
--- a/PVE/Storage/LVMPlugin.pm
+++ b/PVE/Storage/LVMPlugin.pm
@@ -201,6 +201,11 @@ sub type {
return 'lvm';
}
+sub wants_fmt_suffix {
+ my ($class, $scfg) = @_;
+ return 0;
+}
+
Nit: since there is no $scfg->{path}, the default implementation would already
return 0. Same for {RBD,ZFSPool}Plugin below, but overwriting the method doesn't
really hurt either of course.
You're right. I guess it comes down to if we want this to be explicit or
implicit. Other opinions?
sub plugindata {
return {
content => [ {images => 1, rootdir => 1}, { images => 1 }],
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index b1865cb..5c6c659 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -191,6 +191,13 @@ sub default_format {
return wantarray ? ($def_format, $valid_formats) : $def_format;
}
+sub wants_fmt_suffix {
+ my ($class, $scfg) = @_;
+ return 1 if $scfg->{path};
+ return 0;
+}
+
+
PVE::JSONSchema::register_format('pve-storage-path', \&verify_path);
sub verify_path {
my ($path, $noerr) = @_;
diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
index a8d1243..86ea45a 100644
--- a/PVE/Storage/RBDPlugin.pm
+++ b/PVE/Storage/RBDPlugin.pm
@@ -273,6 +273,11 @@ sub type {
return 'rbd';
}
+sub wants_fmt_suffix {
+ my ($class, $scfg) = @_;
+ return 0;
+}
+
sub plugindata {
return {
content => [ {images => 1, rootdir => 1}, { images => 1 }],
diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
index c4be70f..85e2211 100644
--- a/PVE/Storage/ZFSPoolPlugin.pm
+++ b/PVE/Storage/ZFSPoolPlugin.pm
@@ -18,6 +18,11 @@ sub type {
return 'zfspool';
}
+sub wants_fmt_suffix {
+ my ($class, $scfg) = @_;
+ return 0;
+}
+
sub plugindata {
return {
content => [ {images => 1, rootdir => 1}, {images => 1 , rootdir => 1}],
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
