Am 03.08.21 um 16:20 schrieb Aaron Lauterer:
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)?
Yes, that would work too. I'd prefer the latter approach where the
default implementation simply decides based on the presence of
$scfg->{path}.
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.
In principle, there might be external plugins using a format suffix, but
no vmid prefix. Using only one bit of information to decide how the
volname should be is not enough then. But I suppose one can argue that
an external plugin could even have a completely different volname
scheme. Introducing find_free_volname instead would avoid that problem
altogether.
+}
+
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
