12:37 p.m.
The XML-Schema specifies two tags vendor and product for the disk type. But they only
apply to the SCSI bus,
even though the QEMU driver ide-hd and ide-cd have support for the command line argument
-model.
The model corresponds to words 27-46 of the IDENTIFY PACKET DEVICE response from the ATAPI
spec.
Words 27-46 is a 40 Char space for the model number of the device. The model number is
built by
combining the vendor and the product fields with a single space as separator in the
middle.
Therefore I would say, that vendor and product pretty much correlate to the model field in
QEMU,
so the ide disk should also have the possibility of adding vendor and product to it.
The tests got changed to incorporate the new error message which now contains that the ide
and
sata bus also supports vendor and product. Also the xml to command line tests got updated,
to
have both fields present for testing.
The command generator reordered a bit too, to only include disk->vendor and
disk->product into the
json generation, when the scsi bus is selected. The same logic applies to the ide and sata
bus.
Signed-off-by: Benedek Major <benedek(a)major.onl>
---
src/qemu/qemu_command.c | 21 ++++++++++++++++---
src/qemu/qemu_validate.c | 6 ++++--
.../disk-sata-device.x86_64-latest.args | 2 +-
tests/qemuxml2argvdata/disk-sata-device.xml | 2 ++
...csi-disk-vpd-build-error.x86_64-latest.err | 2 +-
.../disk-sata-device.x86_64-latest.xml | 2 ++
6 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ad224571f3..903872091f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1943,8 +1943,6 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
"p:physical_block_size", physical_block_size,
"A:wwn", &wwn,
"p:rotation_rate", disk->rotation_rate,
- "S:vendor", disk->vendor,
- "S:product", disk->product,
"T:removable", removable,
"S:write-cache", qemuOnOffAuto(writeCache),
"p:cyls", disk->geometry.cylinders,
@@ -1956,7 +1954,24 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
"S:rerror", rpolicy,
NULL) < 0)
return NULL;
-
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
+ /* add vendor and product in SCSI section, since only SCSI supports
+ * vendor and product tag*/
+ if (virJSONValueObjectAdd(&props,
+ "S:vendor", disk->vendor,
+ "S:product", disk->product,
+ NULL) < 0)
+ return NULL;
+ }
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE || disk->bus ==
VIR_DOMAIN_DISK_BUS_SATA) {
+ /* Repurpose vendor and product, since IDE-Disk only supports model,
+ * which per ATAPI spec is the combination of vendor and product
+ * separated by a single space*/
+ if (virJSONValueObjectAdd(&props,
+ "S:model", g_strconcat(disk->vendor, " ",
disk->product, NULL),
+ NULL) < 0)
+ return NULL;
+ }
return g_steal_pointer(&props);
}
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 7e09e2c52f..e7312bf789 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -2871,9 +2871,11 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef
*disk,
}
if (disk->vendor || disk->product) {
- if (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) {
+ if (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI
+ && disk->bus != VIR_DOMAIN_DISK_BUS_IDE
+ && disk->bus != VIR_DOMAIN_DISK_BUS_SATA) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Only scsi disk supports vendor and product"));
+ _("Only scsi, sata and ide disk supports vendor and
product"));
return -1;
}
diff --git a/tests/qemuxml2argvdata/disk-sata-device.x86_64-latest.args
b/tests/qemuxml2argvdata/disk-sata-device.x86_64-latest.args
index b60d9b16d4..296f8f1ee0 100644
--- a/tests/qemuxml2argvdata/disk-sata-device.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/disk-sata-device.x86_64-latest.args
@@ -30,7 +30,7 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
-device
'{"driver":"ahci","id":"sata0","bus":"pci.0","addr":"0x2"}'
\
-blockdev
'{"driver":"host_device","filename":"/dev/HostVG/QEMUG
uest1","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}'
\
-blockdev
'{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}'
\
--device
'{"driver":"ide-hd","bus":"sata0.0","drive":"libvirt-1-format","id":"sata0-0-0","bootindex":1}'
\
+-device
'{"driver":"ide-hd","bus":"sata0.0","drive":"libvirt-1-format","id":"sata0-0-0","bootindex":1,"model":"Seagate
ST3500418AS"}' \
-audiodev
'{"id":"audio1","driver":"none"}' \
-device
'{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x3"}'
\
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
diff --git a/tests/qemuxml2argvdata/disk-sata-device.xml
b/tests/qemuxml2argvdata/disk-sata-device.xml
index a9def2923e..51eb472dcc 100644
--- a/tests/qemuxml2argvdata/disk-sata-device.xml
+++ b/tests/qemuxml2argvdata/disk-sata-device.xml
@@ -19,6 +19,8 @@
<source dev='/dev/HostVG/QEMUG uest1'/>
<target dev='sda' bus='sata'/>
<address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ <vendor>Seagate</vendor>
+ <product>ST3500418AS</product>
</disk>
<controller type='usb' index='0'/>
<controller type='sata' index='0'/>
diff --git a/tests/qemuxml2argvdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err
b/tests/qemuxml2argvdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err
index f70b7a774f..fd77b3feb0 100644
--- a/tests/qemuxml2argvdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err
+++ b/tests/qemuxml2argvdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err
@@ -1 +1 @@
-unsupported configuration: Only scsi disk supports vendor and product
+unsupported configuration: Only scsi, sata and ide disk supports vendor and product
diff --git a/tests/qemuxml2xmloutdata/disk-sata-device.x86_64-latest.xml
b/tests/qemuxml2xmloutdata/disk-sata-device.x86_64-latest.xml
index cd20f5185b..6caedf66a5 100644
--- a/tests/qemuxml2xmloutdata/disk-sata-device.x86_64-latest.xml
+++ b/tests/qemuxml2xmloutdata/disk-sata-device.x86_64-latest.xml
@@ -21,6 +21,8 @@
<driver name='qemu' type='raw'/>
<source dev='/dev/HostVG/QEMUG uest1'/>
<target dev='sda' bus='sata'/>
+ <vendor>Seagate</vendor>
+ <product>ST3500418AS</product>
<address type='drive' controller='0' bus='0'
target='0' unit='0'/>
</disk>
<controller type='usb' index='0' model='piix3-uhci'>
--
2.41.0
6:55 a.m.
On Sat, Jul 15, 2023 at 18:37:10 +0200, Benedek Major wrote:
The XML-Schema specifies two tags vendor and product for the disk
type. But they only apply to the SCSI bus,
even though the QEMU driver ide-hd and ide-cd have support for the command line argument
-model.
The model corresponds to words 27-46 of the IDENTIFY PACKET DEVICE response from the
ATAPI spec.
Words 27-46 is a 40 Char space for the model number of the device. The model number is
built by
combining the vendor and the product fields with a single space as separator in the
middle.
If there are any formatting requirements (e.g. from this description it
seems that 'vendor' must not contain any space in order to work, then
you'll need to add code enforcing them.
Therefore I would say, that vendor and product pretty much correlate
to the model field in QEMU,
This feels very subjective. Could you please add a better justification?
E.g. show how qemu formats the default vendor and product name?
so the ide disk should also have the possibility of adding vendor and
product to it.
The tests got changed to incorporate the new error message which now
contains that the ide and
sata bus also supports vendor and product. Also the xml to command line tests got
updated, to
have both fields present for testing.
The command generator reordered a bit too, to only include disk->vendor and
disk->product into the
json generation, when the scsi bus is selected. The same logic applies to the ide and
sata bus.
This bit above can be dropped
Signed-off-by: Benedek Major <benedek(a)major.onl>
---
src/qemu/qemu_command.c | 21 ++++++++++++++++---
src/qemu/qemu_validate.c | 6 ++++--
.../disk-sata-device.x86_64-latest.args | 2 +-
tests/qemuxml2argvdata/disk-sata-device.xml | 2 ++
...csi-disk-vpd-build-error.x86_64-latest.err | 2 +-
.../disk-sata-device.x86_64-latest.xml | 2 ++
6 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ad224571f3..903872091f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1943,8 +1943,6 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
"p:physical_block_size", physical_block_size,
"A:wwn", &wwn,
"p:rotation_rate", disk->rotation_rate,
- "S:vendor", disk->vendor,
- "S:product", disk->product,
"T:removable", removable,
"S:write-cache", qemuOnOffAuto(writeCache),
"p:cyls", disk->geometry.cylinders,
@@ -1956,7 +1954,24 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
"S:rerror", rpolicy,
NULL) < 0)
return NULL;
-
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
+ /* add vendor and product in SCSI section, since only SCSI supports
+ * vendor and product tag*/
+ if (virJSONValueObjectAdd(&props,
+ "S:vendor", disk->vendor,
+ "S:product", disk->product,
Please make your changes conform to the code formatting we use. E.g. the
line above should align all arguments below &props.
Ideally, to prevent code movement, use local variables to extract
vendor/product only if appropriate and initialize them to NULL
+ NULL) < 0)
+ return NULL;
+ }
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE || disk->bus ==
VIR_DOMAIN_DISK_BUS_SATA) {
+ /* Repurpose vendor and product, since IDE-Disk only supports model,
+ * which per ATAPI spec is the combination of vendor and product
+ * separated by a single space*/
+ if (virJSONValueObjectAdd(&props,
+ "S:model", g_strconcat(disk->vendor, " ",
disk->product, NULL),
g_strconcat allocates a new string, but the "S:" json building modifier
does NOT consume it, thus memory is leaked here. You'll need a temporary
variable. This aligns pretty well with the above non-movement of the
code where you can simply add a "S:model", modelstr, into the original
formatter and fill it conditionally.
Additionally the above ine would be misformatted otherwise.
+ NULL) < 0)
+ return NULL;
This also looks misformatted as it's returning if virJSONValueObjectAdd
fails.
+ }
return g_steal_pointer(&props);
}
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 7e09e2c52f..e7312bf789 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -2871,9 +2871,11 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef
*disk,
}
if (disk->vendor || disk->product) {
- if (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) {
+ if (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI
+ && disk->bus != VIR_DOMAIN_DISK_BUS_IDE
+ && disk->bus != VIR_DOMAIN_DISK_BUS_SATA) {
Please follow our coding style, we format logic operators at the end of
the line rather than at the start of the next one.
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s",
- _("Only scsi disk supports vendor and product"));
+ _("Only scsi, sata and ide disk supports vendor and
product"));
return -1;
}
7:11 a.m.
On Sat, Jul 15, 2023 at 06:37:10PM +0200, Benedek Major wrote:
The XML-Schema specifies two tags vendor and product for the disk
type. But they only apply to the SCSI bus,
even though the QEMU driver ide-hd and ide-cd have support for the command line argument
-model.
The model corresponds to words 27-46 of the IDENTIFY PACKET DEVICE response from the
ATAPI spec.
Words 27-46 is a 40 Char space for the model number of the device. The model number is
built by
combining the vendor and the product fields with a single space as separator in the
middle.
Therefore I would say, that vendor and product pretty much correlate to the model field
in QEMU,
so the ide disk should also have the possibility of adding vendor and product to it.
Lets compare SCSI:
https://www.seagate.com/files/staticfiles/support/docs/manual/Interface%2...
[quote]
PRODUCT IDENTIFICATION
The PRODUCT IDENTIFICATION field contains sixteen bytes of left-aligned
ASCII data (see 5.4.2) defined by Seagate. Bytes 16 through 31 indicate
the drive model with 20h (space) used as a filler. The table below is
an example of drive test data returned by the drive. Bytes 16 and 17
will contain 53 54 for all drive models.
[/quote/
And ATARPI
https://people.freebsd.org/~imp/asiabsdcon2015/works/d2161r5-ATAATAPI_Com...
[quote]
A.11.7.4 MODEL NUMBER field
The MODEL NUMBER field contains the model number of the device. The
contents of the MODEL NUMBER field is an ATA string of forty bytes in
the format defined by 3.3.10. The device shall pad the string with
spaces (i.e., 20h), if necessary, to ensure that the string is the
proper length. The combination of the serial number (see A.11.7.2)
and the model number shall be unique for a given manufacturer.
The IDENTIFY DEVICE data contains a copy of the MODEL NUMBER field
(see IDENTIFY DEVICE data words 27..46 in table 45).
[/quote]
my interpretation is that the SCSI "product" field data is identical
to the ATAPI "model" field data.
AFAICT, ATAPI doesn't provide a way to expose a vendor in string
format. So I would say we accept 'product' for IDE, but reject
'vendor'
Having said that it would be interesting to know the results from
an IDENTIFY DEVICE / IDENTIFY PACKET DEVICE command on some *real*
hardware, as an illustration of what vendors actually did in
practice. I don't have access to any real hardware that still
uses SATA/IDE though.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
698
days inactive
700
days old
2 comments
3 participants
participants (3)
-
Benedek Major -
Daniel P. Berrangé -
Peter Krempa