On Mon, Dec 30, 2024 at 21:44:20 +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. > > Resolves: https://gitlab.com/libvirt/libvirt/-/issues/697 > Signed-off-by: Adam Julis <ajulis@xxxxxxxxxx> > --- > > Changes to v1: > - introduce new variables model and product for being consistent in > orders of commands > - modified test file > > docs/formatdomain.rst | 7 ++-- > src/qemu/qemu_command.c | 9 ++++- > 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, 69 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. Looking at the ATAPI spec [1] and qemu code the model for IDE/SATA disks is 40 characters long. Both this documentation and the validation of the virDomainDiskDef 'product' field (in virDomainDiskDefValidate) are wrong when applied to ATA disks. Note that qemu will silently truncate anything longer. Also don't forget to fix the version to 11.1.0. [1] https://people.freebsd.org/~imp/asiabsdcon2015/works/d2161r5-ATAATAPI_Command_Set_-_3.pdf A.11.7.4 MODEL NUMBER field (page 456) > - :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..b6ec2c4a79 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -1619,6 +1619,8 @@ qemuBuildDiskDeviceProps(const virDomainDef *def, > const char *biosCHSTrans = NULL; > const char *wpolicy = NULL; > const char *rpolicy = NULL; > + const char *model = NULL; > + const char *product = NULL; > > switch (disk->bus) { > case VIR_DOMAIN_DISK_BUS_IDE: > @@ -1628,6 +1630,8 @@ qemuBuildDiskDeviceProps(const virDomainDef *def, > else > driver = "ide-hd"; > > + model = disk->product; > + > break; > > case VIR_DOMAIN_DISK_BUS_SCSI: > @@ -1654,6 +1658,8 @@ qemuBuildDiskDeviceProps(const virDomainDef *def, > } > } > > + product = disk->product; > + > break; > > case VIR_DOMAIN_DISK_BUS_VIRTIO: { > @@ -1803,7 +1809,8 @@ qemuBuildDiskDeviceProps(const virDomainDef *def, > "A:wwn", &wwn, > "p:rotation_rate", disk->rotation_rate, > "S:vendor", disk->vendor, > - "S:product", disk->product, > + "S:product", product, > + "S:model", model, > "T:removable", removable, > "S:write-cache", qemuOnOffAuto(writeCache), > "p:cyls", disk->geometry.cylinders, This looks good. > 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")); Since you are changing this please enclose vendor in single quotes. > + 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")); same here for 'product'. > return -1; > } > } [...] > --- a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.args > +++ b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.args Semantically testing 'ide'/'sata' disks doesn't belong to a test case named 'disk-scsi'. Please add another one. > @@ -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 '{"driver":"ide-cd","bus":"ide.0","unit":0,"drive":"libvirt-2-storage","id":"ide0-0-0","model":"ST3146707LC"}' \ > -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 '{"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,"product":"ST67 807GD"}' \ > -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> Also deleting the vendor isn't right. > <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'/>
