* [pve-devel] [PATCH v2 qemu-server] fix #4957: add vendor and product information passthrough for SCSI-Disks
@ 2023-11-08 8:51 Hannes Duerr
2023-11-08 10:04 ` Fiona Ebner
0 siblings, 1 reply; 5+ messages in thread
From: Hannes Duerr @ 2023-11-08 8:51 UTC (permalink / raw)
To: pve-devel
adds vendor and product information for SCSI devices to the json schema and
checks in the VM create/update API call if it is possible to add these to QEMU as a device option
Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
---
changes in v2:
- when calling the API to create/update a VM, check whether the devices
are "scsi-hd" or "scsi-cd" devices,where there is the option to add
vendor and product information, if not error out
- change the format in product_fmt and vendor_fmt to a pattern that only
allows 40 characters consisting of upper and lower case letters, numbers and '-' and '_'.
PVE/API2/Qemu.pm | 9 +++++
PVE/QemuServer.pm | 83 +++++++++++++++++++++++++++++------------
PVE/QemuServer/Drive.pm | 24 ++++++++++++
3 files changed, 92 insertions(+), 24 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 38bdaab..6898ec9 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1030,6 +1030,11 @@ __PACKAGE__->register_method({
);
$conf->{$_} = $created_opts->{$_} for keys $created_opts->%*;
+ foreach my $opt (keys $created_opts->%*) {
+ if ($opt =~ m/scsi/) {
+ PVE::QemuServer::check_scsi_feature_compatibility($opt, $created_opts, $conf, $storecfg, $param);
+ }
+ }
if (!$conf->{boot}) {
my $devs = PVE::QemuServer::get_default_bootdevices($conf);
$conf->{boot} = PVE::QemuServer::print_bootorder($devs);
@@ -1840,6 +1845,10 @@ my $update_vm_api = sub {
);
$conf->{pending}->{$_} = $created_opts->{$_} for keys $created_opts->%*;
+ if ($opt =~ m/scsi/) {
+ PVE::QemuServer::check_scsi_feature_compatibility($opt, $created_opts, $conf, $storecfg, $param);
+ }
+
# default legacy boot order implies all cdroms anyway
if (@bootorder) {
# append new CD drives to bootorder to mark them bootable
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index dbcd568..919728b 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -26,6 +26,7 @@ use Storable qw(dclone);
use Time::HiRes qw(gettimeofday usleep);
use URI::Escape;
use UUID;
+use Data::Dumper;
use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_write_file);
use PVE::CGroup;
@@ -1428,6 +1429,53 @@ my sub get_drive_id {
return "$drive->{interface}$drive->{index}";
}
+sub get_scsi_devicetype {
+ my ($drive, $storecfg, $machine_type) = @_;
+
+ my $devicetype = 'hd';
+ my $path = '';
+ if (drive_is_cdrom($drive)) {
+ $devicetype = 'cd';
+ } else {
+ if ($drive->{file} =~ m|^/|) {
+ $path = $drive->{file};
+ if (my $info = path_is_scsi($path)) {
+ if ($info->{type} == 0 && $drive->{scsiblock}) {
+ $devicetype = 'block';
+ } elsif ($info->{type} == 1) { # tape
+ $devicetype = 'generic';
+ }
+ }
+ } else {
+ $path = PVE::Storage::path($storecfg, $drive->{file});
+ }
+
+ # for compatibility only, we prefer scsi-hd (#2408, #2355, #2380)
+ my $version = kvm_user_version();
+ $version = extract_version($machine_type, $version);
+ if ($path =~ m/^iscsi\:\/\// &&
+ !min_version($version, 4, 1)) {
+ $devicetype = 'generic';
+ }
+ }
+
+ return $devicetype;
+}
+
+sub check_scsi_feature_compatibility {
+ my($opt, $created_opts, $conf, $storecfg, $param) = @_;
+
+ my $drive = parse_drive($opt, $created_opts->{$opt});
+ my $machine_type = get_vm_machine($conf, undef, $conf->{arch});
+ my $drivetype = get_scsi_devicetype($drive, $storecfg, $machine_type);
+
+ if ($drivetype ne 'hd' && $drivetype ne 'cd') {
+ if ($param->{$opt} =~ m/vendor/ || $param->{$opt} =~ m/product/) {
+ die "only 'scsi-hd' and 'scsi-cd' devices support passing vendor and product information\n";
+ }
+ }
+}
+
sub print_drivedevice_full {
my ($storecfg, $conf, $vmid, $drive, $bridges, $arch, $machine_type) = @_;
@@ -1443,31 +1491,8 @@ sub print_drivedevice_full {
my ($maxdev, $controller, $controller_prefix) = scsihw_infos($conf, $drive);
my $unit = $drive->{index} % $maxdev;
- my $devicetype = 'hd';
- my $path = '';
- if (drive_is_cdrom($drive)) {
- $devicetype = 'cd';
- } else {
- if ($drive->{file} =~ m|^/|) {
- $path = $drive->{file};
- if (my $info = path_is_scsi($path)) {
- if ($info->{type} == 0 && $drive->{scsiblock}) {
- $devicetype = 'block';
- } elsif ($info->{type} == 1) { # tape
- $devicetype = 'generic';
- }
- }
- } else {
- $path = PVE::Storage::path($storecfg, $drive->{file});
- }
- # for compatibility only, we prefer scsi-hd (#2408, #2355, #2380)
- my $version = extract_version($machine_type, kvm_user_version());
- if ($path =~ m/^iscsi\:\/\// &&
- !min_version($version, 4, 1)) {
- $devicetype = 'generic';
- }
- }
+ my $devicetype = get_scsi_devicetype($drive, $storecfg, $machine_type);
if (!$conf->{scsihw} || $conf->{scsihw} =~ m/^lsi/ || $conf->{scsihw} eq 'pvscsi') {
$device = "scsi-$devicetype,bus=$controller_prefix$controller.0,scsi-id=$unit";
@@ -1482,6 +1507,16 @@ sub print_drivedevice_full {
}
$device .= ",wwn=$drive->{wwn}" if $drive->{wwn};
+ # only scsi-hd and scsi-cd support passing vendor and product information
+ if ($devicetype eq 'hd' || $devicetype eq 'cd') {
+ if (my $vendor = $drive->{vendor}) {
+ $device .= ",vendor=$vendor";
+ }
+ if (my $product = $drive->{product}) {
+ $device .= ",product=$product";
+ }
+ }
+
} elsif ($drive->{interface} eq 'ide' || $drive->{interface} eq 'sata') {
my $maxdev = ($drive->{interface} eq 'sata') ? $PVE::QemuServer::Drive::MAX_SATA_DISKS : 2;
my $controller = int($drive->{index} / $maxdev);
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index e24ba12..021f182 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -159,6 +159,26 @@ my %iothread_fmt = ( iothread => {
optional => 1,
});
+my %product_fmt = (
+ product => {
+ type => 'string',
+ pattern => '[A-Za-z0-9\-_]{,40}',
+ format_description => 'product',
+ description => "The drive's product name, up to 40 bytes long.",
+ optional => 1,
+ },
+);
+
+my %vendor_fmt = (
+ vendor => {
+ type => 'string',
+ pattern => '[A-Za-z0-9\-_]{,40}',
+ format_description => 'vendor',
+ description => "The drive's vendor name, up to 40 bytes long.",
+ optional => 1,
+ },
+);
+
my %model_fmt = (
model => {
type => 'string',
@@ -281,6 +301,8 @@ my $scsi_fmt = {
%scsiblock_fmt,
%ssd_fmt,
%wwn_fmt,
+ %vendor_fmt,
+ %product_fmt,
};
my $scsidesc = {
optional => 1,
@@ -404,6 +426,8 @@ my $alldrive_fmt = {
%readonly_fmt,
%scsiblock_fmt,
%ssd_fmt,
+ %vendor_fmt,
+ %product_fmt,
%wwn_fmt,
%tpmversion_fmt,
%efitype_fmt,
--
2.39.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH v2 qemu-server] fix #4957: add vendor and product information passthrough for SCSI-Disks
2023-11-08 8:51 [pve-devel] [PATCH v2 qemu-server] fix #4957: add vendor and product information passthrough for SCSI-Disks Hannes Duerr
@ 2023-11-08 10:04 ` Fiona Ebner
2023-11-08 14:28 ` Hannes Dürr
0 siblings, 1 reply; 5+ messages in thread
From: Fiona Ebner @ 2023-11-08 10:04 UTC (permalink / raw)
To: Proxmox VE development discussion, Hannes Duerr
Am 08.11.23 um 09:51 schrieb Hannes Duerr:
> adds vendor and product information for SCSI devices to the json schema and
> checks in the VM create/update API call if it is possible to add these to QEMU as a device option
>
> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
> ---
>
> changes in v2:
> - when calling the API to create/update a VM, check whether the devices
> are "scsi-hd" or "scsi-cd" devices,where there is the option to add
> vendor and product information, if not error out
> - change the format in product_fmt and vendor_fmt to a pattern that only
> allows 40 characters consisting of upper and lower case letters, numbers and '-' and '_'.
>
> PVE/API2/Qemu.pm | 9 +++++
> PVE/QemuServer.pm | 83 +++++++++++++++++++++++++++++------------
> PVE/QemuServer/Drive.pm | 24 ++++++++++++
> 3 files changed, 92 insertions(+), 24 deletions(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 38bdaab..6898ec9 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -1030,6 +1030,11 @@ __PACKAGE__->register_method({
> );
> $conf->{$_} = $created_opts->{$_} for keys $created_opts->%*;
>
> + foreach my $opt (keys $created_opts->%*) {
Style nit: new code should use for instead of foreach
Maybe sort the keys to have a deterministic order.
> + if ($opt =~ m/scsi/) {
> + PVE::QemuServer::check_scsi_feature_compatibility($opt, $created_opts, $conf, $storecfg, $param);
Style nit: line too long (100 characters is our limit)
Note that $created_opts was already merged into $conf two lines above.
I'd rather not introduce new usage of that variable.
Can we do the check before creating the drive instead? We know if it's a
CD or pass-through and the path or if it's iscsi ahead of time and that
should be enough for the check, or what am I missing?
> + }
> + }
> if (!$conf->{boot}) {
> my $devs = PVE::QemuServer::get_default_bootdevices($conf);
> $conf->{boot} = PVE::QemuServer::print_bootorder($devs);
> @@ -1840,6 +1845,10 @@ my $update_vm_api = sub {
> );
> $conf->{pending}->{$_} = $created_opts->{$_} for keys $created_opts->%*;
>
> + if ($opt =~ m/scsi/) {
> + PVE::QemuServer::check_scsi_feature_compatibility($opt, $created_opts, $conf, $storecfg, $param);
> + }
> +
> # default legacy boot order implies all cdroms anyway
> if (@bootorder) {
> # append new CD drives to bootorder to mark them bootable
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index dbcd568..919728b 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -26,6 +26,7 @@ use Storable qw(dclone);
> use Time::HiRes qw(gettimeofday usleep);
> use URI::Escape;
> use UUID;
> +use Data::Dumper;
>
Left-over from debugging ;)? You can also use
use JSON;
print to_json($whatever, { canonical => 1, pretty => 1 });
which is often nicer IMHO
> use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_write_file);
> use PVE::CGroup;
> @@ -1428,6 +1429,53 @@ my sub get_drive_id {
> return "$drive->{interface}$drive->{index}";
> }
>
Can you please split the patch in two? A preparatory ones for creating
this helper and another one for the feature.
I also think the helper would be nicer in the QemuServer/Drive.pm
module, but would need more preparation first to avoid a cyclic
dependency. That is:
* change the function to take $machine_version instead of $machine_type
* move the path_is_scsi and the scsi_inquiry function called by that
also to the drive module.
> +sub get_scsi_devicetype {
> + my ($drive, $storecfg, $machine_type) = @_;
It's unfortunate that this needs both $storecfg and $machine_type just
for that compatibility check for the rather old machine version. But I
guess there's not much we can do at the moment.
> +
> + my $devicetype = 'hd';
> + my $path = '';
> + if (drive_is_cdrom($drive)) {
> + $devicetype = 'cd';
> + } else {
> + if ($drive->{file} =~ m|^/|) {
> + $path = $drive->{file};
> + if (my $info = path_is_scsi($path)) {
> + if ($info->{type} == 0 && $drive->{scsiblock}) {
> + $devicetype = 'block';
> + } elsif ($info->{type} == 1) { # tape
> + $devicetype = 'generic';
> + }
> + }
> + } else {
> + $path = PVE::Storage::path($storecfg, $drive->{file});
> + }
> +
> + # for compatibility only, we prefer scsi-hd (#2408, #2355, #2380)
> + my $version = kvm_user_version();
> + $version = extract_version($machine_type, $version);
Why was this changed during the move? It's one line more now. Also, this
can be changed
> + if ($path =~ m/^iscsi\:\/\// &&
> + !min_version($version, 4, 1)) {
> + $devicetype = 'generic';
> + }
> + }
> +
> + return $devicetype;
> +}
> +
> +sub check_scsi_feature_compatibility {
This is rather an assert than a check, because it will die
> + my($opt, $created_opts, $conf, $storecfg, $param) = @_;
Style nit: missing space after my
This is a rather "heavy" signature. Can we get away with less? I.e. just
pass in the new drive (string), $storecfg and stuff for machine_type.
> +
> + my $drive = parse_drive($opt, $created_opts->{$opt});
> + my $machine_type = get_vm_machine($conf, undef, $conf->{arch});
> + my $drivetype = get_scsi_devicetype($drive, $storecfg, $machine_type);
> +
> + if ($drivetype ne 'hd' && $drivetype ne 'cd') {
> + if ($param->{$opt} =~ m/vendor/ || $param->{$opt} =~ m/product/) {
> + die "only 'scsi-hd' and 'scsi-cd' devices support passing vendor and product information\n";
> + }
> + }
> +}
> +
(...)
> @@ -281,6 +301,8 @@ my $scsi_fmt = {
> %scsiblock_fmt,
> %ssd_fmt,
> %wwn_fmt,
> + %vendor_fmt,
> + %product_fmt,
Nit: rest is ordered alphabetically, new ones not
> };
> my $scsidesc = {
> optional => 1,
> @@ -404,6 +426,8 @@ my $alldrive_fmt = {
> %readonly_fmt,
> %scsiblock_fmt,
> %ssd_fmt,
> + %vendor_fmt,
> + %product_fmt,
> %wwn_fmt,
> %tpmversion_fmt,
> %efitype_fmt,
Nit: rest is mostly ordered alphabetically, new ones make it worse
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH v2 qemu-server] fix #4957: add vendor and product information passthrough for SCSI-Disks
2023-11-08 10:04 ` Fiona Ebner
@ 2023-11-08 14:28 ` Hannes Dürr
2023-11-08 14:43 ` Thomas Lamprecht
2023-11-09 8:34 ` Fiona Ebner
0 siblings, 2 replies; 5+ messages in thread
From: Hannes Dürr @ 2023-11-08 14:28 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 11/8/23 11:04, Fiona Ebner wrote:
> Am 08.11.23 um 09:51 schrieb Hannes Duerr:
>> adds vendor and product information for SCSI devices to the json schema and
>> checks in the VM create/update API call if it is possible to add these to QEMU as a device option
>>
>> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
>> ---
>>
>> changes in v2:
>> - when calling the API to create/update a VM, check whether the devices
>> are "scsi-hd" or "scsi-cd" devices,where there is the option to add
>> vendor and product information, if not error out
>> - change the format in product_fmt and vendor_fmt to a pattern that only
>> allows 40 characters consisting of upper and lower case letters, numbers and '-' and '_'.
>>
>> PVE/API2/Qemu.pm | 9 +++++
>> PVE/QemuServer.pm | 83 +++++++++++++++++++++++++++++------------
>> PVE/QemuServer/Drive.pm | 24 ++++++++++++
>> 3 files changed, 92 insertions(+), 24 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 38bdaab..6898ec9 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -1030,6 +1030,11 @@ __PACKAGE__->register_method({
>> );
>> $conf->{$_} = $created_opts->{$_} for keys $created_opts->%*;
>>
>> + foreach my $opt (keys $created_opts->%*) {
> Style nit: new code should use for instead of foreach
>
> Maybe sort the keys to have a deterministic order.
>
>> + if ($opt =~ m/scsi/) {
>> + PVE::QemuServer::check_scsi_feature_compatibility($opt, $created_opts, $conf, $storecfg, $param);
>
> Style nit: line too long (100 characters is our limit)
>
> Note that $created_opts was already merged into $conf two lines above.
> I'd rather not introduce new usage of that variable.
>
> Can we do the check before creating the drive instead? We know if it's a
> CD or pass-through and the path or if it's iscsi ahead of time and that
> should be enough for the check, or what am I missing?
I don't think its possible to check in advance as the config can still
contain a not properly formed path like:
'local-lvm:5', which will be formed to the real path when creating the
disk or am I mistaken ?
>> + }
>> + }
>> if (!$conf->{boot}) {
>> my $devs = PVE::QemuServer::get_default_bootdevices($conf);
>> $conf->{boot} = PVE::QemuServer::print_bootorder($devs);
>> @@ -1840,6 +1845,10 @@ my $update_vm_api = sub {
>> );
>> $conf->{pending}->{$_} = $created_opts->{$_} for keys $created_opts->%*;
>>
>> + if ($opt =~ m/scsi/) {
>> + PVE::QemuServer::check_scsi_feature_compatibility($opt, $created_opts, $conf, $storecfg, $param);
>> + }
>> +
>> # default legacy boot order implies all cdroms anyway
>> if (@bootorder) {
>> # append new CD drives to bootorder to mark them bootable
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index dbcd568..919728b 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -26,6 +26,7 @@ use Storable qw(dclone);
>> use Time::HiRes qw(gettimeofday usleep);
>> use URI::Escape;
>> use UUID;
>> +use Data::Dumper;
>>
> Left-over from debugging ;)? You can also use
> use JSON;
> print to_json($whatever, { canonical => 1, pretty => 1 });
> which is often nicer IMHO
thanks for the hint, I'll have a look 🙂
>> use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_write_file);
>> use PVE::CGroup;
>> @@ -1428,6 +1429,53 @@ my sub get_drive_id {
>> return "$drive->{interface}$drive->{index}";
>> }
>>
> Can you please split the patch in two? A preparatory ones for creating
> this helper and another one for the feature.
>
> I also think the helper would be nicer in the QemuServer/Drive.pm
> module, but would need more preparation first to avoid a cyclic
> dependency. That is:
> * change the function to take $machine_version instead of $machine_type
> * move the path_is_scsi and the scsi_inquiry function called by that
> also to the drive module.
>
>> +sub get_scsi_devicetype {
>> + my ($drive, $storecfg, $machine_type) = @_;
> It's unfortunate that this needs both $storecfg and $machine_type just
> for that compatibility check for the rather old machine version. But I
> guess there's not much we can do at the moment.
>
>> +
>> + my $devicetype = 'hd';
>> + my $path = '';
>> + if (drive_is_cdrom($drive)) {
>> + $devicetype = 'cd';
>> + } else {
>> + if ($drive->{file} =~ m|^/|) {
>> + $path = $drive->{file};
>> + if (my $info = path_is_scsi($path)) {
>> + if ($info->{type} == 0 && $drive->{scsiblock}) {
>> + $devicetype = 'block';
>> + } elsif ($info->{type} == 1) { # tape
>> + $devicetype = 'generic';
>> + }
>> + }
>> + } else {
>> + $path = PVE::Storage::path($storecfg, $drive->{file});
>> + }
>> +
>> + # for compatibility only, we prefer scsi-hd (#2408, #2355, #2380)
>> + my $version = kvm_user_version();
>> + $version = extract_version($machine_type, $version);
> Why was this changed during the move? It's one line more now. Also, this
> can be changed
>
>> + if ($path =~ m/^iscsi\:\/\// &&
>> + !min_version($version, 4, 1)) {
>> + $devicetype = 'generic';
>> + }
>> + }
>> +
>> + return $devicetype;
>> +}
>> +
>> +sub check_scsi_feature_compatibility {
> This is rather an assert than a check, because it will die
>
>> + my($opt, $created_opts, $conf, $storecfg, $param) = @_;
> Style nit: missing space after my
>
> This is a rather "heavy" signature. Can we get away with less? I.e. just
> pass in the new drive (string), $storecfg and stuff for machine_type.
>
>> +
>> + my $drive = parse_drive($opt, $created_opts->{$opt});
>> + my $machine_type = get_vm_machine($conf, undef, $conf->{arch});
>> + my $drivetype = get_scsi_devicetype($drive, $storecfg, $machine_type);
>> +
>> + if ($drivetype ne 'hd' && $drivetype ne 'cd') {
>> + if ($param->{$opt} =~ m/vendor/ || $param->{$opt} =~ m/product/) {
>> + die "only 'scsi-hd' and 'scsi-cd' devices support passing vendor and product information\n";
>> + }
>> + }
>> +}
>> +
> (...)
what does that mean ?
>> @@ -281,6 +301,8 @@ my $scsi_fmt = {
>> %scsiblock_fmt,
>> %ssd_fmt,
>> %wwn_fmt,
>> + %vendor_fmt,
>> + %product_fmt,
> Nit: rest is ordered alphabetically, new ones not
>
>> };
>> my $scsidesc = {
>> optional => 1,
>> @@ -404,6 +426,8 @@ my $alldrive_fmt = {
>> %readonly_fmt,
>> %scsiblock_fmt,
>> %ssd_fmt,
>> + %vendor_fmt,
>> + %product_fmt,
>> %wwn_fmt,
>> %tpmversion_fmt,
>> %efitype_fmt,
> Nit: rest is mostly ordered alphabetically, new ones make it worse
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH v2 qemu-server] fix #4957: add vendor and product information passthrough for SCSI-Disks
2023-11-08 14:28 ` Hannes Dürr
@ 2023-11-08 14:43 ` Thomas Lamprecht
2023-11-09 8:34 ` Fiona Ebner
1 sibling, 0 replies; 5+ messages in thread
From: Thomas Lamprecht @ 2023-11-08 14:43 UTC (permalink / raw)
To: Proxmox VE development discussion, Hannes Dürr, Fiona Ebner
Am 08/11/2023 um 15:28 schrieb Hannes Dürr:
> On 11/8/23 11:04, Fiona Ebner wrote:
>> Am 08.11.23 um 09:51 schrieb Hannes Duerr:
>>> + if ($opt =~ m/scsi/) {
>>> + PVE::QemuServer::check_scsi_feature_compatibility($opt, $created_opts, $conf, $storecfg, $param);
>>
>> Style nit: line too long (100 characters is our limit)
>>
>> Note that $created_opts was already merged into $conf two lines above.
>> I'd rather not introduce new usage of that variable.
>>
>> Can we do the check before creating the drive instead? We know if it's a
>> CD or pass-through and the path or if it's iscsi ahead of time and that
>> should be enough for the check, or what am I missing?
> I don't think its possible to check in advance as the config can still
> contain a not properly formed path like:
> 'local-lvm:5', which will be formed to the real path when creating the
> disk or am I mistaken ?
But all information is still there? I.e., the disk's bus, like scsi, and if any
vendor or product properties are set. So you can still parse that value and check
validity.
>>> + }
>>> + }
>>> if (!$conf->{boot}) {
>>> my $devs = PVE::QemuServer::get_default_bootdevices($conf);
>>> $conf->{boot} = PVE::QemuServer::print_bootorder($devs);
>> (...)
> what does that mean ?
That fiona snipped (trimmed) some context due to it not being relevant for
the reveiw, like in quotes, albeit they more often use brackets there, like
"[...] lorem ipsum"
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH v2 qemu-server] fix #4957: add vendor and product information passthrough for SCSI-Disks
2023-11-08 14:28 ` Hannes Dürr
2023-11-08 14:43 ` Thomas Lamprecht
@ 2023-11-09 8:34 ` Fiona Ebner
1 sibling, 0 replies; 5+ messages in thread
From: Fiona Ebner @ 2023-11-09 8:34 UTC (permalink / raw)
To: Hannes Dürr, Proxmox VE development discussion
Am 08.11.23 um 15:28 schrieb Hannes Dürr:
>>
>> Can we do the check before creating the drive instead? We know if it's a
>> CD or pass-through and the path or if it's iscsi ahead of time and that
>> should be enough for the check, or what am I missing?
> I don't think its possible to check in advance as the config can still
> contain a not properly formed path like:
> 'local-lvm:5', which will be formed to the real path when creating the
> disk or am I mistaken
You can just teach the get_scsi_devicetype() helper to recognize the
special syntax and compare by storage type in that case (can/should be
it's own patch ;)). At a glance, 'iscsidirect' and 'zfs' are the ones
using an "iscsi://" path, but please double check. Of course there could
be a third-party plugin using "iscsi://" too, but this is just an early
check and only applies to VMs with machine version < 4.1 anyways. These
cases should be very rare and it'll just fail later so no big deal. The
benefit of having the check earlier for all other cases is enough to
justify that IMHO.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-11-09 8:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-08 8:51 [pve-devel] [PATCH v2 qemu-server] fix #4957: add vendor and product information passthrough for SCSI-Disks Hannes Duerr
2023-11-08 10:04 ` Fiona Ebner
2023-11-08 14:28 ` Hannes Dürr
2023-11-08 14:43 ` Thomas Lamprecht
2023-11-09 8:34 ` Fiona Ebner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox