From: Daniel Kral <d.kral@proxmox.com>
To: Fiona Ebner <f.ebner@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [RFC qemu-server 3/9] fix #5284: move_vm: add check if target storage supports vm images
Date: Wed, 22 Jan 2025 14:18:33 +0100 [thread overview]
Message-ID: <396f29b5-f9f3-47d7-bcf4-6e52c94c60c4@proxmox.com> (raw)
In-Reply-To: <97e9a3eb-0a4c-4fe1-90e9-8f9b2cb78fa7@proxmox.com>
On 11/29/24 15:23, Fiona Ebner wrote:
> For issues like these, it's often nice to start out with the fix and put
> bigger refactorings later. Then the fix can already be applied up-front
> while discussing the bigger changes.
ACK, I'll move this in front of the other changes.
>
> Am 16.09.24 um 18:38 schrieb Daniel Kral:
>> diff --git a/PVE/QemuServer/Helpers.pm b/PVE/QemuServer/Helpers.pm
>> index 9d0f24aa..a5f6b328 100644
>> --- a/PVE/QemuServer/Helpers.pm
>> +++ b/PVE/QemuServer/Helpers.pm
>> @@ -11,6 +11,8 @@ use PVE::ProcFSTools;
>>
>> use base 'Exporter';
>> our @EXPORT_OK = qw(
>> +check_storage_alloc
>> +check_volume_alloc
>> min_version
>> config_aware_timeout
>> parse_number_sets
>> @@ -151,6 +153,50 @@ sub check_volume_content_type : prototype($$) {
>> return check_storage_content_type($storecfg, $storeid, $vtype);
>> }
>>
>> +=head3 check_storage_alloc($rpcenv, $user, $storeid)
>> +
>> +Checks whether the C<$user> has the permissions in the C<$rpcenv> to allocate space in the storage
>> +with the identifier C<$storeid>.
>> +
>> +
>> +If the check fails, the subroutine will C<die> with a permission exception inside the subroutine
>> +L<PVE::RPCEnvironment::check>.
>> +
>> +Returns C<1> if the check is successful.
>> +
>> +=cut
>> +
>> +sub check_storage_alloc : prototype($$$) {
>
> I'd rather call it assert_storage_alloc_permission
>
>> + my ($rpcenv, $user, $storeid) = @_;
>> +
>> + if (defined($rpcenv) && defined($user)) {
>
> Should we rather assert these? It should not be called in a context
> where we don't have them. In fact, I'd prefer this to be a private
> helper in the API module directly. But I'm not fully convinced we need a
> helper for this to begin with, the actual code is just two lines (or one
> statement).
I agree with all of your points, I'm also in favor of just dropping the
helper in general for v2.
>
>> + $rpcenv->check($user, "/storage/$storeid", ['Datastore.AllocateSpace'])
>> + if $user ne 'root@pam';
>> + }
>> +
>> + return 1;
>> +}
>> +
>> +=head3 check_volume_alloc($storecfg, $storeid, $node)
>> +
>> +Checks whether the volume with the identifier C<$volid>, that is defined in C<$storecfg> (which
>> +is typically retrieved with L<PVE::Storage::config>), is enabled an supports volume images.
>> +
>> +If the check fails, it will C<die> with an error message.
>> +
>> +Returns C<1> if the check is successful.
>> +
>> +=cut
>> +
>> +sub check_volume_alloc : prototype($$;$) {
>
> Again, "assert_" and "_permission"
Hm, why did you choose permission here?
There are no permission checks done here, would be a suffix like
"_allowed" or "_available" also be fine for you?
>
> should/could also live in the storage library as it does not depend on
> anything else
ACK
>
>> + my ($storecfg, $storeid, $node) = @_;
>> +
>> + PVE::Storage::storage_check_enabled($storecfg, $storeid, $node);
>> + check_storage_content_type($storecfg, $storeid);
>> +
>> + return 1;
>> +}
>> +
>> sub min_version {
>> my ($verstr, $major, $minor, $pve) = @_;
>>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
next prev parent reply other threads:[~2025-01-22 13:19 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-09-16 16:38 [pve-devel] [RFC qemu-server 0/9] consistent checks for storage content types on volume disk allocation Daniel Kral
2024-09-16 16:38 ` [pve-devel] [RFC qemu-server 1/9] test: cfg2cmd: expect error for invalid volume's storage content type Daniel Kral
2024-11-29 14:23 ` Fiona Ebner
2024-09-16 16:38 ` [pve-devel] [RFC qemu-server 2/9] cfg2cmd: improve error message for invalid volume " Daniel Kral
2024-11-29 14:23 ` Fiona Ebner
2025-01-22 13:16 ` Daniel Kral
2025-01-22 14:31 ` Fiona Ebner
2024-09-16 16:38 ` [pve-devel] [RFC qemu-server 3/9] fix #5284: move_vm: add check if target storage supports vm images Daniel Kral
2024-11-29 14:23 ` Fiona Ebner
2025-01-22 13:18 ` Daniel Kral [this message]
2025-01-22 13:43 ` Daniel Kral
2025-01-22 14:35 ` Fiona Ebner
2024-09-16 16:38 ` [pve-devel] [RFC qemu-server 4/9] api: clone_vm: add check if " Daniel Kral
2024-11-29 14:23 ` Fiona Ebner
2025-01-22 13:18 ` Daniel Kral
2024-09-16 16:38 ` [pve-devel] [RFC qemu-server 5/9] api: create_vm: improve checks if storages for disks support " Daniel Kral
2024-09-16 16:38 ` [pve-devel] [RFC qemu-server 6/9] cloudinit: add check if storage for cloudinit disk supports " Daniel Kral
2024-09-16 16:38 ` [pve-devel] [RFC qemu-server 7/9] api: migrate_vm: improve check if target storages support " Daniel Kral
2024-11-29 14:23 ` Fiona Ebner
2025-01-22 13:19 ` Daniel Kral
2024-09-16 16:38 ` [pve-devel] [RFC qemu-server 8/9] api: importdisk: improve check if storage supports " Daniel Kral
2024-09-16 16:38 ` [pve-devel] [RFC qemu-server 9/9] restore_vm: improve checks " Daniel Kral
2024-11-29 14:23 ` Fiona Ebner
2025-01-22 13:21 ` Daniel Kral
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=396f29b5-f9f3-47d7-bcf4-6e52c94c60c4@proxmox.com \
--to=d.kral@proxmox.com \
--cc=f.ebner@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