On Tue, Jul 18, 2023 at 02:30:44 +0200, Benedek Major wrote:
> This is the patch, as suggested by Daniel, using only the product tag for IDE/SATA.
> Since qemu supports the model tag, if the drive bus is IDE OR SATA the product string gets passed to qemu using the model parameter
> If the bus is either SATA or IDE, the product string can have a max lenght of 40, as per ATAPI spec in the field "model" of the
> IDENTIFY PACKET DEVICE. This should be the better solution, since you do not need to rely on space checking in the vendor field.
> Tell me what you think about this version, and if it is satisfactory, I will write the missing tests.
>
> Thanks for your time and effort,
> Benedek
Since the commit message is always kept in the history of git, please
use it just to describe the technicalities of the patch itself, but not
anything to the actual contribution process itself.
>
> Signed-off-by: Benedek Major <benedek@xxxxxxxxx>
> ---
> src/conf/domain_validate.c | 18 +++++++++++++++---
> src/qemu/qemu_command.c | 10 ++++++++--
> src/qemu/qemu_validate.c | 12 +++++++++++-
> 3 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 16bf3b559f..b9940c78a5 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -585,7 +585,8 @@ virDomainDiskDefValidateSource(const virStorageSource *src)
>
>
> #define VENDOR_LEN 8
> -#define PRODUCT_LEN 16
> +#define PRODUCT_LEN_SCSI 16
> +#define PRODUCT_LEN_IDE 40
>
>
> /**
> @@ -856,10 +857,21 @@ virDomainDiskDefValidate(const virDomainDef *def,
> return -1;
> }
>
> - if (strlen(disk->product) > PRODUCT_LEN) {
> + if ((disk->bus == VIR_DOMAIN_DISK_BUS_SATA ||
> + disk->bus == VIR_DOMAIN_DISK_BUS_IDE) &&
The above line is misaligned, both 'disk->bus' should start on the same
column as they are part of the same sub-condition
> + strlen(disk->product) > PRODUCT_LEN_IDE) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("disk product is more than %1$d characters"),
> - PRODUCT_LEN);
> + PRODUCT_LEN_IDE);
> + return -1;
> + }
> +
> + if (disk->bus != VIR_DOMAIN_DISK_BUS_SATA &&
> + disk->bus != VIR_DOMAIN_DISK_BUS_IDE &&
> + strlen(disk->product) > PRODUCT_LEN_SCSI) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("disk product is more than %1$d characters"),
> + PRODUCT_LEN_SCSI);
> return -1;
> }
> }
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ad224571f3..7bc1d8c682 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1762,6 +1762,8 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
> unsigned int physical_block_size = disk->blockio.physical_block_size;
> g_autoptr(virJSONValue) wwn = NULL;
> g_autofree char *serial = NULL;
> + g_autofree char *modelstr = NULL;
> + g_autofree char *product = g_strdup(disk->product);
You are not modifying any of the strings so there's no need to copy it.
Rather declare both variables as const char *, keep the asignment of
disk->product into the helper 'product'
> virTristateSwitch removable = VIR_TRISTATE_SWITCH_ABSENT;
> virTristateSwitch writeCache = VIR_TRISTATE_SWITCH_ABSENT;
> const char *biosCHSTrans = NULL;
> @@ -1775,7 +1777,10 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
> driver = "ide-cd";
> else
> driver = "ide-hd";
> -
> + /* exchange product to model for QEMU ide disk */
/* IDE disk product name for IDE is passed via 'model' property */
modelstr = g_steal_pointer(&product);
break;
> + modelstr = g_strdup(product);
> + g_free(product);
> + product = NULL;
> break;
>
> case VIR_DOMAIN_DISK_BUS_SCSI:
> @@ -1944,7 +1949,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", modelstr,
> "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 7e09e2c52f..4893b979d3 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -2871,12 +2871,22 @@ 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->vendor) {
Format this condition as you did below:
if (disk->vendor &&
disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) {
You can then also drop the 'if (disk->vendor || disk->product) {' block
as it will become redundant once ...
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("Only scsi disk supports vendor and product"));
> return -1;
> }
>
> + if (disk->product &&
> + 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 scsi, ide, sata disk supports product"));
> + return -1;
> + }
> +
> +
> /* Properties wwn, vendor and product were introduced in the
> * same QEMU release (1.2.0).
> */
... I'll post a patch to remove this dead code. Minimum supported qemu
is 4.2.0, thus we always have this.
Once you do the above, you can add:
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
I'll post the final version, after you mainly fix the commit message.
