8:04 a.m.
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
8:09 a.m.
New subject: [PATCH] Added support for vendor and product for qemu ide-hd
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 :|
7:30 p.m.
New subject: [PATCH] Added support for vendor and product for qemu ide-hd
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(a)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
8:43 a.m.
New subject: [PATCH] Added support for vendor and product for qemu ide-hd
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(a)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(a)redhat.com>
I'll post the final version, after you mainly fix the commit message.
3:39 p.m.
New subject: [PATCH] Added support for disk model string for QEMU IDE and SATA devices
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(a)major.onl>
Reviewed-by: Peter Krempa <pkrempa(a)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
5:36 a.m.
New subject: [PATCH] Added support for disk model string for QEMU IDE and SATA devices
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(a)major.onl>
Reviewed-by: Peter Krempa <pkrempa(a)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(-)
528
days inactive
530
days old
5 comments
3 participants
participants (3)
-
Benedek Major -
Daniel P. Berrangé -
Peter Krempa