On Sat, Jan 18, 2025 at 11:30: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. Length of the 'product' parameter is
> different in SCSI (16 chars) and ATA/SATA (40 chars).
>
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/697
> Signed-off-by: Adam Julis <[email protected]>
> ---
>
> Changes to v2:
> - doc (mainly mentioned the different length of SCSI and SATA/ATA)
> - modified schemas (for supporting longer string)
> - added tests
> - ..details in formatting
>
> docs/formatdomain.rst | 9 ++--
> src/conf/domain_validate.c | 14 +++++--
> src/conf/schemas/domaincommon.rng | 2 +-
> src/qemu/qemu_command.c | 9 +++-
> src/qemu/qemu_validate.c | 14 ++++++-
> .../disk-sata-product.x86_64-latest.args | 36 ++++++++++++++++
> .../disk-sata-product.x86_64-latest.xml | 41 +++++++++++++++++++
> tests/qemuxmlconfdata/disk-sata-product.xml | 28 +++++++++++++
> ...csi-disk-vpd-build-error.x86_64-latest.err | 2 +-
> ...disk-scsi-product-length.x86_64-latest.err | 1 +
> .../disk-scsi-product-length.xml | 28 +++++++++++++
> tests/qemuxmlconftest.c | 2 +
> 12 files changed, 173 insertions(+), 13 deletions(-)
> create mode 100644 tests/qemuxmlconfdata/disk-sata-product.x86_64-latest.args
> create mode 100644 tests/qemuxmlconfdata/disk-sata-product.x86_64-latest.xml
> create mode 100644 tests/qemuxmlconfdata/disk-sata-product.xml
> create mode 100644
> tests/qemuxmlconfdata/disk-scsi-product-length.x86_64-latest.err
> create mode 100644 tests/qemuxmlconfdata/disk-scsi-product-length.xml
>
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 620daae9af..0544b11fb9 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -3562,12 +3562,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
> + '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`
> + CD-ROM device. It must not be longer than 16 printable characters for
> 'scsi'
> + (:since:`Since 1.0.1`). For 'sata' or 'ide' not longer than 40 printable
> + characters (:since:`Since 11.0.1`). Other ``bus`` is not supported.
11.1.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/conf/domain_validate.c b/src/conf/domain_validate.c
> index 6aed74dd8d..01dda3a278 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -603,8 +603,8 @@ virDomainDiskDefValidateSource(const virStorageSource
> *src)
>
>
> #define VENDOR_LEN 8
> -#define PRODUCT_LEN 16
> -
> +#define PRODUCT_SCSI_LEN 16
> +#define PRODUCT_ATA_SATA_LEN 40
>
> /**
> * virDomainDiskDefSourceLUNValidate:
> @@ -880,10 +880,16 @@ virDomainDiskDefValidate(const virDomainDef *def,
> return -1;
> }
>
> - if (strlen(disk->product) > PRODUCT_LEN) {
> + if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI &&
> + strlen(disk->product) > PRODUCT_SCSI_LEN) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("disk product is more than %1$d characters"),
> - PRODUCT_LEN);
> + PRODUCT_SCSI_LEN);
> + return -1;
> + } else if (strlen(disk->product) > PRODUCT_ATA_SATA_LEN) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("disk product is more than %1$d characters"),
> + PRODUCT_ATA_SATA_LEN);
> return -1;
> }
> }
I think I'll change this to add a temporary variable for the length
rather than duplicate the whole code.
[...]
I'll change the two things and push this.
Reviewed-by: Peter Krempa <[email protected]>
