1. Patchew
  2. Libvirt
  3. View series

[PATCH] Support IDE/SATA disk 'product' parameter

Adam Julis posted 1 patch 1 week, 3 days ago 
docs/formatdomain.rst                         |  7 ++--
src/qemu/qemu_command.c                       | 11 +++++-
src/qemu/qemu_validate.c                      | 14 ++++++--
...disk-product-build-error.x86_64-latest.err |  1 +
.../disk-scsi-disk-product-build-error.xml    | 34 +++++++++++++++++++
...-disk-vendor-build-error.x86_64-latest.err |  1 +
... => disk-scsi-disk-vendor-build-error.xml} |  0
...csi-disk-vpd-build-error.x86_64-latest.err |  1 -
.../disk-scsi-disk-vpd.x86_64-latest.args     |  4 +--
.../disk-scsi-disk-vpd.x86_64-latest.xml      |  7 ++--
tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml  |  4 +--
tests/qemuxmlconftest.c                       |  3 +-
12 files changed, 71 insertions(+), 16 deletions(-)
create mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.xml
create mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.x86_64-latest.err
rename tests/qemuxmlconfdata/{disk-scsi-disk-vpd-build-error.xml => disk-scsi-disk-vendor-build-error.xml} (100%)
delete mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err
[PATCH] Support IDE/SATA disk 'product' parameter
Posted by Adam Julis 1 week, 3 days ago 
Since we supported 'product' parameter for SCSI, just expanded existing
solution makes IDE/SATA parameter works too. QEMU requires parameter 'model'
in case of IDE/SATA (instead of 'product'), so the process of making JSON
object is slightly modified for that.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/697
Signed-off-by: Adam Julis <ajulis@redhat.com>
---
 docs/formatdomain.rst                         |  7 ++--
 src/qemu/qemu_command.c                       | 11 +++++-
 src/qemu/qemu_validate.c                      | 14 ++++++--
 ...disk-product-build-error.x86_64-latest.err |  1 +
 .../disk-scsi-disk-product-build-error.xml    | 34 +++++++++++++++++++
 ...-disk-vendor-build-error.x86_64-latest.err |  1 +
 ... => disk-scsi-disk-vendor-build-error.xml} |  0
 ...csi-disk-vpd-build-error.x86_64-latest.err |  1 -
 .../disk-scsi-disk-vpd.x86_64-latest.args     |  4 +--
 .../disk-scsi-disk-vpd.x86_64-latest.xml      |  7 ++--
 tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml  |  4 +--
 tests/qemuxmlconftest.c                       |  3 +-
 12 files changed, 71 insertions(+), 16 deletions(-)
 create mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.x86_64-latest.err
 create mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.xml
 create mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.x86_64-latest.err
 rename tests/qemuxmlconfdata/{disk-scsi-disk-vpd-build-error.xml => disk-scsi-disk-vendor-build-error.xml} (100%)
 delete mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 60bee8bd4f..c93a321401 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -3551,12 +3551,13 @@ paravirtualized driver is specified via the ``disk`` element.
    :since:`Since 0.10.1`
 ``vendor``
    If present, this element specifies the vendor of a virtual hard disk or
-   CD-ROM device. It must not be longer than 8 printable characters.
-   :since:`Since 1.0.1`
+   CD-ROM device. It must not be longer than 8 printable characters. Only for
+   devices using 'scsi' ``bus``. :since:`Since 1.0.1`
 ``product``
    If present, this element specifies the product of a virtual hard disk or
    CD-ROM device. It must not be longer than 16 printable characters.
-   :since:`Since 1.0.1`
+   Only for devices using 'scsi' (:since:`Since 1.0.1`), 'sata' or 'ide' ``bus``.
+   :since:`Since 11.0.0`
 ``address``
    If present, the ``address`` element ties the disk to a given slot of a
    controller (the actual ``<controller>`` device can often be inferred by
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index dcb9c4934e..5c38858f5d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1628,6 +1628,11 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
         else
             driver = "ide-hd";
 
+        if (virJSONValueObjectAdd(&props,
+                                 "S:model", disk->product,
+                                 NULL) < 0)
+            return NULL;
+
         break;
 
     case VIR_DOMAIN_DISK_BUS_SCSI:
@@ -1654,6 +1659,11 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
             }
         }
 
+        if (virJSONValueObjectAdd(&props,
+                                 "S:product", disk->product,
+                                 NULL) < 0)
+            return NULL;
+
         break;
 
     case VIR_DOMAIN_DISK_BUS_VIRTIO: {
@@ -1803,7 +1813,6 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
                               "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,
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index aaa056379e..f0be236533 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -2947,10 +2947,20 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk,
         }
     }
 
-    if (disk->vendor || disk->product) {
+    if (disk->vendor) {
         if (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("Only scsi disk supports vendor and product"));
+                           _("Only scsi disk supports vendor"));
+            return -1;
+        }
+    }
+
+    if (disk->product) {
+        if ((disk->bus != VIR_DOMAIN_DISK_BUS_IDE) &&
+            (disk->bus != VIR_DOMAIN_DISK_BUS_SATA) &&
+            (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("Only ide, sata and scsi disk supports product"));
             return -1;
         }
     }
diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.x86_64-latest.err b/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.x86_64-latest.err
new file mode 100644
index 0000000000..93dfac0d1e
--- /dev/null
+++ b/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.x86_64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: Only ide, sata and scsi disk supports product
diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.xml b/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.xml
new file mode 100644
index 0000000000..da2fc59da3
--- /dev/null
+++ b/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.xml
@@ -0,0 +1,34 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219136</memory>
+  <currentMemory unit='KiB'>219136</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <disk type='block' device='disk'>
+      <source dev='/dev/HostVG/QEMUGuest1'/>
+      <target dev='sda' bus='virtio'/>
+      <product>ST3146707LC</product>
+    </disk>
+    <disk type='block' device='disk'>
+      <source dev='/dev/HostVG/QEMUGuest2'/>
+      <target dev='sdb' bus='scsi'/>
+      <vendor>SEAGATE</vendor>
+      <product>ST3567807GD</product>
+      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+    </disk>
+    <controller type='usb' index='0'/>
+    <controller type='scsi' index='0' model='virtio-scsi'/>
+    <controller type='scsi' index='1' model='lsilogic'/>
+    <memballoon model='virtio'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.x86_64-latest.err b/tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.x86_64-latest.err
new file mode 100644
index 0000000000..88bd9e5468
--- /dev/null
+++ b/tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.x86_64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: Only scsi disk supports vendor
diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.xml b/tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.xml
similarity index 100%
rename from tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.xml
rename to tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.xml
diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err b/tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err
deleted file mode 100644
index f70b7a774f..0000000000
--- a/tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err
+++ /dev/null
@@ -1 +0,0 @@
-unsupported configuration: Only scsi disk supports vendor and product
diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.args b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.args
index 4234a7e677..1d3aaf3819 100644
--- a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.args
+++ b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.args
@@ -30,9 +30,9 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
 -device '{"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.0","addr":"0x2"}' \
 -device '{"driver":"lsi","id":"scsi1","bus":"pci.0","addr":"0x3"}' \
 -blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-2-storage","read-only":true}' \
--device '{"driver":"scsi-cd","bus":"scsi0.0","channel":0,"scsi-id":0,"lun":0,"device_id":"drive-scsi0-0-0-0","drive":"libvirt-2-storage","id":"scsi0-0-0-0","vendor":"SEAGATE","product":"ST3146707LC"}' \
+-device '{"model":"ST3146707LC","driver":"ide-cd","bus":"ide.0","unit":0,"drive":"libvirt-2-storage","id":"ide0-0-0"}' \
 -blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest2","node-name":"libvirt-1-storage","read-only":true}' \
--device '{"driver":"scsi-hd","bus":"scsi1.0","scsi-id":0,"device_id":"drive-scsi1-0-0","drive":"libvirt-1-storage","id":"scsi1-0-0","bootindex":1,"vendor":"SEA GATE","product":"ST67 807GD"}' \
+-device '{"product":"ST67 807GD","driver":"scsi-hd","bus":"scsi1.0","scsi-id":0,"device_id":"drive-scsi1-0-0","drive":"libvirt-1-storage","id":"scsi1-0-0","bootindex":1}' \
 -audiodev '{"id":"audio1","driver":"none"}' \
 -device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x4"}' \
 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.xml b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.xml
index 4b23fbfcfe..39148f6ce7 100644
--- a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.xml
+++ b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.xml
@@ -20,9 +20,8 @@
     <disk type='block' device='cdrom'>
       <driver name='qemu' type='raw'/>
       <source dev='/dev/HostVG/QEMUGuest1'/>
-      <target dev='sda' bus='scsi'/>
+      <target dev='sda' bus='ide'/>
       <readonly/>
-      <vendor>SEAGATE</vendor>
       <product>ST3146707LC</product>
       <address type='drive' controller='0' bus='0' target='0' unit='0'/>
     </disk>
@@ -31,7 +30,6 @@
       <source dev='/dev/HostVG/QEMUGuest2'/>
       <target dev='sdb' bus='scsi'/>
       <readonly/>
-      <vendor>SEA GATE</vendor>
       <product>ST67 807GD</product>
       <address type='drive' controller='1' bus='0' target='0' unit='0'/>
     </disk>
@@ -45,6 +43,9 @@
       <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
     </controller>
     <controller type='pci' index='0' model='pci-root'/>
+    <controller type='ide' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+    </controller>
     <input type='mouse' bus='ps2'/>
     <input type='keyboard' bus='ps2'/>
     <audio id='1' type='none'/>
diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml
index 36dd2a89ba..e3665d3afa 100644
--- a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml
+++ b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml
@@ -16,9 +16,8 @@
     <emulator>/usr/bin/qemu-system-x86_64</emulator>
     <disk type='block' device='cdrom'>
       <source dev='/dev/HostVG/QEMUGuest1'/>
-      <target dev='sda' bus='scsi'/>
+      <target dev='sda' bus='ide'/>
       <readonly/>
-      <vendor>SEAGATE</vendor>
       <product>ST3146707LC</product>
       <address type='drive' controller='0' bus='0' target='0' unit='0'/>
     </disk>
@@ -26,7 +25,6 @@
       <source dev='/dev/HostVG/QEMUGuest2'/>
       <target dev='sdb' bus='scsi'/>
       <readonly/>
-      <vendor>SEA GATE</vendor>
       <product>ST67 807GD</product>
       <address type='drive' controller='1' bus='0' target='0' unit='0'/>
     </disk>
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 21b56dc94e..083b0ab7f6 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -1620,7 +1620,8 @@ mymain(void)
     DO_TEST_CAPS_LATEST("disk-scsi-disk-split");
     DO_TEST_CAPS_LATEST("disk-scsi-disk-wwn");
     DO_TEST_CAPS_LATEST("disk-scsi-disk-vpd");
-    DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-scsi-disk-vpd-build-error");
+    DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-scsi-disk-vendor-build-error");
+    DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-scsi-disk-product-build-error");
     DO_TEST_CAPS_LATEST("controller-virtio-scsi");
     DO_TEST_CAPS_LATEST("controller-scsi-auto");
     DO_TEST_CAPS_LATEST("disk-sata-device");
-- 
2.47.0
Re: [PATCH] Support IDE/SATA disk 'product' parameter
Posted by Peter Krempa 1 week, 3 days ago 
On Fri, Dec 20, 2024 at 14:30:42 +0100, Adam Julis wrote:
> Since we supported 'product' parameter for SCSI, just expanded existing
> solution makes IDE/SATA parameter works too. QEMU requires parameter 'model'
> in case of IDE/SATA (instead of 'product'), so the process of making JSON
> object is slightly modified for that.
> 
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/697
> Signed-off-by: Adam Julis <ajulis@redhat.com>
> ---
>  docs/formatdomain.rst                         |  7 ++--
>  src/qemu/qemu_command.c                       | 11 +++++-
>  src/qemu/qemu_validate.c                      | 14 ++++++--
>  ...disk-product-build-error.x86_64-latest.err |  1 +
>  .../disk-scsi-disk-product-build-error.xml    | 34 +++++++++++++++++++
>  ...-disk-vendor-build-error.x86_64-latest.err |  1 +
>  ... => disk-scsi-disk-vendor-build-error.xml} |  0
>  ...csi-disk-vpd-build-error.x86_64-latest.err |  1 -
>  .../disk-scsi-disk-vpd.x86_64-latest.args     |  4 +--
>  .../disk-scsi-disk-vpd.x86_64-latest.xml      |  7 ++--
>  tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml  |  4 +--
>  tests/qemuxmlconftest.c                       |  3 +-
>  12 files changed, 71 insertions(+), 16 deletions(-)
>  create mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.x86_64-latest.err
>  create mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.xml
>  create mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.x86_64-latest.err
>  rename tests/qemuxmlconfdata/{disk-scsi-disk-vpd-build-error.xml => disk-scsi-disk-vendor-build-error.xml} (100%)
>  delete mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err

Disclaimer: Not a full review. I'll do one if nobody picks this up until
I'm working again.

> @@ -30,9 +30,9 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
>  -device '{"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.0","addr":"0x2"}' \
>  -device '{"driver":"lsi","id":"scsi1","bus":"pci.0","addr":"0x3"}' \
>  -blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-2-storage","read-only":true}' \
> --device '{"driver":"scsi-cd","bus":"scsi0.0","channel":0,"scsi-id":0,"lun":0,"device_id":"drive-scsi0-0-0-0","drive":"libvirt-2-storage","id":"scsi0-0-0-0","vendor":"SEAGATE","product":"ST3146707LC"}' \
> +-device '{"model":"ST3146707LC","driver":"ide-cd","bus":"ide.0","unit":0,"drive":"libvirt-2-storage","id":"ide0-0-0"}' \
>  -blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest2","node-name":"libvirt-1-storage","read-only":true}' \
> --device '{"driver":"scsi-hd","bus":"scsi1.0","scsi-id":0,"device_id":"drive-scsi1-0-0","drive":"libvirt-1-storage","id":"scsi1-0-0","bootindex":1,"vendor":"SEA GATE","product":"ST67 807GD"}' \
> +-device '{"product":"ST67 807GD","driver":"scsi-hd","bus":"scsi1.0","scsi-id":0,"device_id":"drive-scsi1-0-0","drive":"libvirt-1-storage","id":"scsi1-0-0","bootindex":1}' \

Do not reorder these. 'driver' should always stay first for readability.

>  -audiodev '{"id":"audio1","driver":"none"}' \
>  -device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x4"}' \
>  -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.