From: Fabian Ebner <f.ebner@proxmox.com>
To: pve-devel@lists.proxmox.com, Aaron Lauterer <a.lauterer@proxmox.com>
Subject: Re: [pve-devel] [PATCH v1 storage 1/9] storage: expose find_free_diskname
Date: Mon, 2 Aug 2021 14:56:54 +0200 [thread overview]
Message-ID: <6619be2d-d8df-c1f4-e633-c6b37a184de7@proxmox.com> (raw)
In-Reply-To: <20210719145254.2269142-2-a.lauterer@proxmox.com>
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.lauterer@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).
> +}
> +
> 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.
> 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}],
>
next prev parent reply other threads:[~2021-08-02 12:57 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-19 14:52 [pve-devel] [PATCH v1 storage qemu-server container 0/9] move disk or volume to other guests Aaron Lauterer
2021-07-19 14:52 ` [pve-devel] [PATCH v1 storage 1/9] storage: expose find_free_diskname Aaron Lauterer
2021-08-02 12:56 ` Fabian Ebner [this message]
2021-08-03 14:20 ` Aaron Lauterer
2021-08-04 7:27 ` Fabian Ebner
2021-07-19 14:52 ` [pve-devel] [PATCH v1 storage 2/9] add disk rename feature Aaron Lauterer
2021-08-02 12:57 ` Fabian Ebner
2021-08-03 14:22 ` Aaron Lauterer
2021-07-19 14:52 ` [pve-devel] [PATCH v1 qemu-server 3/9] cli: qm: change move_disk to move-disk Aaron Lauterer
2021-07-19 14:52 ` [pve-devel] [PATCH v1 qemu-server 4/9] Drive: add valid_drive_names_with_unused Aaron Lauterer
2021-08-03 7:35 ` Fabian Ebner
2021-07-19 14:52 ` [pve-devel] [PATCH v1 qemu-server 5/9] api: move-disk: add move to other VM Aaron Lauterer
2021-08-03 7:34 ` Fabian Ebner
2021-08-05 9:36 ` Aaron Lauterer
2021-07-19 14:52 ` [pve-devel] [PATCH v1 qemu-server 6/9] api: move-disk: cleanup very long lines Aaron Lauterer
2021-08-03 7:37 ` Fabian Ebner
2021-07-19 14:52 ` [pve-devel] [PATCH v1 container 7/9] cli: pct: change move_volume to move-volume Aaron Lauterer
2021-07-19 14:52 ` [pve-devel] [PATCH v1 container 8/9] api: move-volume: add move to another container Aaron Lauterer
2021-08-03 8:14 ` Fabian Ebner
2021-08-05 12:45 ` Aaron Lauterer
2021-07-19 14:52 ` [pve-devel] [PATCH v1 container 9/9] api: move-volume: cleanup very long lines Aaron Lauterer
2021-08-03 8:27 ` [pve-devel] [PATCH v1 storage qemu-server container 0/9] move disk or volume to other guests Fabian Ebner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6619be2d-d8df-c1f4-e633-c6b37a184de7@proxmox.com \
--to=f.ebner@proxmox.com \
--cc=a.lauterer@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal