Re: [PATCH] Added support for vendor and product for qemu ide-hd
Hello, thanks for your fast responses. I've also recognized flaws in my implementation, after your comments, so I'll try to find a solution, that is acceptable, and create a better patch. But first, I'll need to know if this approach is correct:
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' I would also say so. Therefore I would need to expand the vendor field to 40 Chars, if a SATA/IDE bus is selected. (current is 8 for SCSI).
about this:
an IDENTIFY DEVICE / IDENTIFY PACKET DEVICE command on some *real* hardware With hdparm -I /dev/sdX I got: Model Number: TOSHIBA DT01ACA050 Model Number: Samsung SSD 850 EVO 250GB
I think hdparam just dumps the raw responses, but correct me if I'm wrong.
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? QEMU formats the defaults as "QEMU HARDDISK" and "QEMU CD-ROM" This was just an assumption, for which I didn't find any spec yet. Since every disk I know is in the form of: MANUFACTURER SOME OTHER MODEL NAME Maybe it is just a convention, but I have no experience in that field. In the Windows VM in device manager under the SATA device for "QEMU HARDDISK" it shows as Device instance path SCSI\DISK&VEN_QEMU&PROD_HARDDISK\, which also made me think that it is the norm.
About the coding style, I'll fix those problems. And thanks for reminding me of the memory leaks in C, im so accustomed to writing in code that does not have them, that I almost forgot how aware you must be of declaring pointers. Sincerely, Benedek
On Mon, Jul 17, 2023 at 03:04:56PM +0200, Benedek Major wrote:
Hello, thanks for your fast responses. I've also recognized flaws in my implementation, after your comments, so I'll try to find a solution, that is acceptable, and create a better patch. But first, I'll need to know if this approach is correct:
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' I would also say so. Therefore I would need to expand the vendor field to 40 Chars, if a SATA/IDE bus is selected. (current is 8 for SCSI).
about this:
an IDENTIFY DEVICE / IDENTIFY PACKET DEVICE command on some *real* hardware With hdparm -I /dev/sdX I got: Model Number: TOSHIBA DT01ACA050 Model Number: Samsung SSD 850 EVO 250GB
I think hdparam just dumps the raw responses, but correct me if I'm wrong.
I've checked the source and you're correct, this the exact model string. So it seems in practice the model contains both the vendor and product strings, which validates your proposal to do the same in libvirt. 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 :|
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 Signed-off-by: Benedek Major <benedek@major.onl> --- 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) && + 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); 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 */ + 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) { 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). */ -- 2.41.0
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@major.onl> --- 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@redhat.com> I'll post the final version, after you mainly fix the commit message.
The QEMU drivers ide-hd and ide-cd used for IDE and SATA bus devices support adding a disk model that is returned on guest query by the IDE or SATA disk to the guest. The already existing product tag got changed to allow for up to 40 characters, which is the maximum allowed by the specification, if the controller is set to SATA or IDE. If set, the product gets passed to QEMU using the -model parameter. The product represents the model until command creation, where it gets processed depending on the current type of disk controller. Dead code checking for QEMU version 1.2.0 got removed, since minimum is 4.2.0. Signed-off-by: Benedek Major <benedek@major.onl> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_validate.c | 18 +++++++++++++++--- src/qemu/qemu_command.c | 8 ++++++-- src/qemu/qemu_validate.c | 28 +++++++++++++--------------- 3 files changed, 34 insertions(+), 20 deletions(-) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 16bf3b559f..60454f3745 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) && + 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..a3b0380695 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; + const char *modelstr = NULL; + const char *product = disk->product; virTristateSwitch removable = VIR_TRISTATE_SWITCH_ABSENT; virTristateSwitch writeCache = VIR_TRISTATE_SWITCH_ABSENT; const char *biosCHSTrans = NULL; @@ -1775,7 +1777,8 @@ qemuBuildDiskDeviceProps(const virDomainDef *def, driver = "ide-cd"; else driver = "ide-hd"; - + /* IDE disk product name for IDE is passed via 'model' property */ + modelstr = g_steal_pointer(&product); break; case VIR_DOMAIN_DISK_BUS_SCSI: @@ -1944,7 +1947,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..84f9af3f16 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -2870,22 +2870,20 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk, } } - if (disk->vendor || disk->product) { - if (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only scsi disk supports vendor and product")); - return -1; - } + if (disk->vendor && + disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Only scsi disk supports vendor and product")); + return -1; + } - /* Properties wwn, vendor and product were introduced in the - * same QEMU release (1.2.0). - */ - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_DISK_WWN)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Setting vendor or product for scsi disk is not " - "supported by this QEMU")); - 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; } if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { -- 2.41.0
On Tue, Jul 18, 2023 at 22:39:26 +0200, Benedek Major wrote:
The QEMU drivers ide-hd and ide-cd used for IDE and SATA bus devices support adding a disk model that is returned on guest query by the IDE or SATA disk to the guest. The already existing product tag got changed to allow for up to 40 characters, which is the maximum allowed by the specification, if the controller is set to SATA or IDE. If set, the product gets passed to QEMU using the -model parameter. The product represents the model until command creation, where it gets processed depending on the current type of disk controller.
Dead code checking for QEMU version 1.2.0 got removed, since minimum is 4.2.0.
I've posted proper patches to do this part properly as I've originally replied in the review. I'll rebase this patch on top of that series so that it's done properly. I'll also also adjust the commit message to go along with that, so there's no need to resend.
Signed-off-by: Benedek Major <benedek@major.onl> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_validate.c | 18 +++++++++++++++--- src/qemu/qemu_command.c | 8 ++++++-- src/qemu/qemu_validate.c | 28 +++++++++++++--------------- 3 files changed, 34 insertions(+), 20 deletions(-)
participants (3)
-
Benedek Major -
Daniel P. Berrangé -
Peter Krempa