From: Fabian Ebner <f.ebner@proxmox.com>
To: pve-devel@lists.proxmox.com,
"Fabian Grünbichler" <f.gruenbichler@proxmox.com>
Subject: Re: [pve-devel] [PATCH v12 qemu-server 15/16] api: support VM disk import
Date: Wed, 16 Mar 2022 10:29:08 +0100 [thread overview]
Message-ID: <834cda55-7463-bb21-eb32-da3d2bc514cd@proxmox.com> (raw)
In-Reply-To: <1647267065.5tbtb00mlw.astroid@nora.none>
Am 14.03.22 um 16:54 schrieb Fabian Grünbichler:
> On March 9, 2022 11:09 am, Fabian Ebner wrote:
---snip---
>> my $check_drive_param = sub {
>> my ($param, $storecfg, $extra_checks) = @_;
>>
>> for my $opt (sort keys $param->%*) {
>> next if !PVE::QemuServer::is_valid_drivename($opt);
>>
>> - my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
>> + my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
>
> technically belongs into the previous patch, our non-alloc schema is
> just tolerant enough because it doesn't look at the volids too closely
> and accepts the NEW_DISK_RE syntax as potential existing volid..
>
Makes sense. I guess I wanted to keep the other patch minimal, but
there's no good reason for that.
>> raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
>>
>> + if ($drive->{'import-from'}) {
>> + die "'import-from' requires special syntax - use <storage ID>:0,import-from=<source>\n"
>> + if $drive->{file} !~ $NEW_DISK_RE || $3 != 0;
>
> should probably be a param_exc
>
>> +
>> + if ($opt eq 'efidisk0') {
>> + for my $required (qw(efitype pre-enrolled-keys)) {
>> + die "$opt - need to specify '$required' when using 'import-from'\n"
>> + if !defined($drive->{$required});
>
> same here
>
>> + }
>> + } elsif ($opt eq 'tpmstate0') {
>> + die "$opt - need to specify 'version' when using 'import-from'\n"
>> + if !defined($drive->{version});
>
> and here
>
Will change it.
>> + }
>> + }
>> +
>> PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive);
>>
>> $extra_checks->($drive) if $extra_checks;
>>
>> - $param->{$opt} = PVE::QemuServer::print_drive($drive);
>> + $param->{$opt} = PVE::QemuServer::print_drive($drive, 1);
>> }
>> };
>>
>> -my $NEW_DISK_RE = qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!;
>> my $check_storage_access = sub {
>> my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage) = @_;
>>
>> - PVE::QemuConfig->foreach_volume($settings, sub {
>> + $foreach_volume_with_alloc->($settings, sub {
>> my ($ds, $drive) = @_;
>>
>> my $isCDROM = PVE::QemuServer::drive_is_cdrom($drive);
>> @@ -106,6 +137,20 @@ my $check_storage_access = sub {
>> } else {
>> PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>> }
>> +
>> + if (my $src_image = $drive->{'import-from'}) {
>> + my $src_vmid;
>> + my ($src_storeid) = PVE::Storage::parse_volume_id($src_image, 1);
>> + if ($src_storeid) { # PVE-managed volume
>
> nit, could be
>
> if (PVE::Storage::parse_volume_id($src_image, 1)) { # PVE-managed
>
> since we don't actually care about the sid here, and parse_volume_id
> will return undef when $noerr is set.
>
>> + $src_vmid = (PVE::Storage::parse_volname($storecfg, $src_image))[2]
>
> is there some case where we expect parse_volume_id to work, but the
> volume to not have an associated guest? because perl doesn't mind us
> accessing the resulting array at arbitrary indices, so this doesn't fail
> if $src_vmid is undef..
>
Yes, when importing from an iscsi storage (not sure if there's other
cases). The check below and $import_from_volid both handle the case
where $src_vmid is undef.
> these should probably also check some more stuff (at least the volume
> type?) - else we get strange errors when attempting to import
> non-image-volumes (some of which even have owners, for example backup
> archives..), and what exactly gets caught where is basically up to the
> storage plugin via parse_volname and volume_has_feature..
Will add a check for vtype.
>
>> + }
>> +
>> + if ($src_vmid) { # might be actively used by VM and will be copied via clone_disk()
>> + $rpcenv->check($authuser, "/vms/${src_vmid}", ['VM.Clone']);
>> + } else {
>> + PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $src_image);
>> + }
>> + }
>> });
>>
>> $rpcenv->check($authuser, "/storage/$settings->{vmstatestorage}", ['Datastore.AllocateSpace'])
>> @@ -164,6 +209,87 @@ my $check_storage_access_migrate = sub {
>> if !$scfg->{content}->{images};
>> };
>>
>> +my $import_from_volid = sub {
>> + my ($storecfg, $src_volid, $dest_info, $vollist) = @_;
>> +
>> + die "cannot import from cloudinit disk\n"
>> + if PVE::QemuServer::Drive::drive_is_cloudinit({ file => $src_volid });
>> +
>> + my ($src_storeid, $src_volname) = PVE::Storage::parse_volume_id($src_volid);
>
> technically this is already implied by the sub's name, we checked it
> already outside, but we need the store id for the bwlimit below..
>
Yes, it's not intended to be a check, although if it does fail something
went terribly wrong and it's good that we abort ;) I'll move it closer
to where it's actually used and I'll drop the unused $src_volname.
---snip---
>> + if (my $source = delete $disk->{'import-from'}) {
>> + my $dst_volid;
>> + my ($src_storeid) = PVE::Storage::parse_volume_id($source, 1);
>> +
>> + if ($src_storeid) { # PVE-managed volume
>
> same as above applies here as well, $src_storeid is not used here, so
> can be shortened.
>
>> + die "could not get size of $source\n"
>> + if !PVE::Storage::volume_size_info($storecfg, $source, 10);
>
> this could move into $import_from_volid?
Will do.
---snip---
>> @@ -242,7 +415,7 @@ my $create_disks = sub {
>> }
>> };
>>
>> - eval { PVE::QemuConfig->foreach_volume($settings, $code); };
>> + eval { $foreach_volume_with_alloc->($settings, $code); };
>>
>> # free allocated images on error
>> if (my $err = $@) {
>> @@ -1285,7 +1458,7 @@ my $update_vm_api = sub {
>>
>> my $check_drive_perms = sub {
>> my ($opt, $val) = @_;
>> - my $drive = PVE::QemuServer::parse_drive($opt, $val);
>> + my $drive = PVE::QemuServer::parse_drive($opt, $val, 1);
>
> same applies here (move to previous patch?)
>
>> # FIXME: cloudinit: CDROM or Disk?
>> if (PVE::QemuServer::drive_is_cdrom($drive)) { # CDROM
>> $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
>> @@ -1391,7 +1564,7 @@ my $update_vm_api = sub {
>> # default legacy boot order implies all cdroms anyway
>> if (@bootorder) {
>> # append new CD drives to bootorder to mark them bootable
>> - my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
>> + my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
>
> same
>
---snip---
>> @@ -547,7 +566,7 @@ sub drive_is_read_only {
>> # [,iothread=on][,serial=serial][,model=model]
>>
>> sub parse_drive {
>> - my ($key, $data) = @_;
>> + my ($key, $data, $with_alloc) = @_;
>
> technically previous patch, same as all the other changes in this file
> below this change
>
Ack.
next prev parent reply other threads:[~2022-03-16 9:29 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-09 10:09 [pve-devel] [PATCH-SERIES v12 qemu-server/manager] API for disk import and OVF Fabian Ebner
2022-03-09 10:09 ` [pve-devel] [PATCH v12 qemu-server 01/16] device unplug: verify that unplugging scsi disk completed Fabian Ebner
2022-03-09 10:09 ` [pve-devel] [PATCH v12 qemu-server 02/16] api: create disks: always activate/update size when attaching existing volume Fabian Ebner
2022-03-09 10:09 ` [pve-devel] [PATCH v12 qemu-server 03/16] api: update: pass correct config when creating disks Fabian Ebner
2022-03-09 10:09 ` [pve-devel] [PATCH v12 qemu-server 04/16] clone disk: remove check for min QEMU version 2.7 Fabian Ebner
2022-03-09 10:09 ` [pve-devel] [PATCH v12 qemu-server 05/16] clone disk: group source and target parameters Fabian Ebner
2022-03-09 10:09 ` [pve-devel] [PATCH v12 qemu-server 06/16] clone disk: pass in efi vars size rather than config Fabian Ebner
2022-03-09 10:09 ` [pve-devel] [PATCH v12 qemu-server 07/16] clone disk: allow cloning from an unused or unreferenced disk Fabian Ebner
[not found] ` <<20220309100919.31512-8-f.ebner@proxmox.com>
2022-03-14 15:55 ` Fabian Grünbichler
2022-03-17 10:35 ` Fabian Ebner
2022-03-09 10:09 ` [pve-devel] [PATCH v12 qemu-server 08/16] efivars size: allow overriding efidisk parameter Fabian Ebner
2022-03-09 10:09 ` [pve-devel] [PATCH v12 qemu-server 09/16] schema: add pve-volume-id-or-absolute-path Fabian Ebner
2022-03-09 10:09 ` [pve-devel] [PATCH v12 qemu-server 10/16] parse ovf: untaint path when calling file_size_info Fabian Ebner
2022-03-09 10:09 ` [pve-devel] [PATCH v12 qemu-server 11/16] api: add endpoint for parsing .ovf files Fabian Ebner
[not found] ` <<20220309100919.31512-12-f.ebner@proxmox.com>
2022-03-14 15:55 ` Fabian Grünbichler
2022-03-15 13:00 ` Fabian Ebner
2022-03-09 10:09 ` [pve-devel] [PATCH v12 qemu-server 12/16] image convert: allow block device as source Fabian Ebner
2022-03-09 10:09 ` [pve-devel] [PATCH v12 qemu-server 13/16] api: factor out check/cleanup for drive params Fabian Ebner
2022-03-09 10:09 ` [pve-devel] [PATCH v12 qemu-server 14/16] schema: drive: use separate schema when disk allocation is possible Fabian Ebner
2022-03-09 10:09 ` [pve-devel] [PATCH v12 qemu-server 15/16] api: support VM disk import Fabian Ebner
[not found] ` <<20220309100919.31512-16-f.ebner@proxmox.com>
2022-03-14 15:54 ` Fabian Grünbichler
2022-03-16 9:29 ` Fabian Ebner [this message]
2022-03-09 10:09 ` [pve-devel] [PATCH v12 qemu-server 16/16] api: update vm: print drive string for newly allocated/imported drives Fabian Ebner
2022-03-09 10:09 ` [pve-devel] [PATCH v12 manager 1/1] api: nodes: add readovf endpoint Fabian Ebner
2022-03-14 15:57 ` [pve-devel] partially-applied: [PATCH-SERIES v12 qemu-server/manager] API for disk import and OVF Fabian Grünbichler
2022-03-16 10:00 ` Fabian Ebner
2022-03-16 10:29 ` Fabian Grünbichler
2022-03-16 11:25 ` Fabian Ebner
2022-03-16 11:58 ` Fabian Grünbichler
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=834cda55-7463-bb21-eb32-da3d2bc514cd@proxmox.com \
--to=f.ebner@proxmox.com \
--cc=f.gruenbichler@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox