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.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
