[libvirt] [PATCH] qemu: Allow the user to specify vendor and product for disk
QEMU supports to set vendor and product strings for disk since 1.2.0 (only scsi-disk, scsi-hd, scsi-cd support it), this patch exposes it with new XML elements <vendor> and <product> of disk device. --- docs/formatdomain.html.in | 10 +++++ docs/schemas/domaincommon.rng | 10 +++++ src/conf/domain_conf.c | 30 ++++++++++++++++ src/conf/domain_conf.h | 2 + src/qemu/qemu_command.c | 29 ++++++++++++++++ .../qemuxml2argv-disk-scsi-disk-vpd.args | 13 +++++++ .../qemuxml2argv-disk-scsi-disk-vpd.xml | 36 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++ 8 files changed, 134 insertions(+), 0 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c8da33d..cc9e871 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1657,6 +1657,16 @@ of 16 hexadecimal digits. <span class='since'>Since 0.10.1</span> </dd> + <dt><code>vendor</code></dt> + <dd>If present, this element specifies the vendor of a virtual hard + disk or CD-ROM device. It's a not more than 8 bytes alphanumeric string. + <span class='since'>Since 1.0.1</span> + </dd> + <dt><code>product</code></dt> + <dd>If present, this element specifies the product of a virtual hard + disk or CD-ROM device. It's a not more than 16 bytes alphanumeric string. + <span class='since'>Since 1.0.1</span> + </dd> <dt><code>host</code></dt> <dd>The <code>host</code> element has two attributes "name" and "port", which specify the hostname and the port number. The meaning of this diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 2beb035..ed7d1d0 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -905,6 +905,16 @@ <ref name="wwn"/> </element> </optional> + <optional> + <element name="vendor"> + <text/> + </element> + </optional> + <optional> + <element name="product"> + <text/> + </element> + </optional> </interleave> </define> <define name="snapshot"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0575fcd..db6608e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -979,6 +979,8 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def->mirror); VIR_FREE(def->auth.username); VIR_FREE(def->wwn); + VIR_FREE(def->vendor); + VIR_FREE(def->product); if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE) VIR_FREE(def->auth.secret.usage); virStorageEncryptionFree(def->encryption); @@ -3498,6 +3500,8 @@ cleanup: goto cleanup; } +#define VENDOR_LEN 8 +#define PRODUCT_LEN 16 /* Parse the XML definition for a disk * @param node XML nodeset to parse for disk definition @@ -3550,6 +3554,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *logical_block_size = NULL; char *physical_block_size = NULL; char *wwn = NULL; + char *vendor = NULL; + char *product = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -3888,6 +3894,24 @@ virDomainDiskDefParseXML(virCapsPtr caps, if (!virValidateWWN(wwn)) goto error; + } else if (!vendor && + xmlStrEqual(cur->name, BAD_CAST "vendor")) { + vendor = (char *)xmlNodeGetContent(cur); + + if (strlen(vendor) > VENDOR_LEN) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("disk vendor is more than 8 bytes string")); + goto error; + } + } else if (!product && + xmlStrEqual(cur->name, BAD_CAST "product")) { + product = (char *)xmlNodeGetContent(cur); + + if (strlen(vendor) > PRODUCT_LEN) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("disk product is more than 16 bytes string")); + goto error; + } } else if (xmlStrEqual(cur->name, BAD_CAST "boot")) { /* boot is parsed as part of virDomainDeviceInfoParseXML */ } @@ -4184,6 +4208,10 @@ virDomainDiskDefParseXML(virCapsPtr caps, serial = NULL; def->wwn = wwn; wwn = NULL; + def->vendor = vendor; + vendor = NULL; + def->product = product; + product = NULL; if (driverType) { def->format = virStorageFileFormatTypeFromString(driverType); @@ -4257,6 +4285,8 @@ cleanup: VIR_FREE(logical_block_size); VIR_FREE(physical_block_size); VIR_FREE(wwn); + VIR_FREE(vendor); + VIR_FREE(product); ctxt->node = save_ctxt; return def; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6539281..c7c1ca6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -591,6 +591,8 @@ struct _virDomainDiskDef { char *serial; char *wwn; + char *vendor; + char *product; int cachemode; int error_policy; /* enum virDomainDiskErrorPolicy */ int rerror_policy; /* enum virDomainDiskErrorPolicy */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 389c480..b0b81f3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2428,6 +2428,13 @@ qemuBuildDriveDevStr(virDomainDefPtr def, } } + if ((disk->vendor || disk->product) && + disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Only scsi disk support vendor and product")); + goto error; + } + if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { /* make sure that both the bus and the qemu binary support * type='lun' (SG_IO). @@ -2455,6 +2462,11 @@ qemuBuildDriveDevStr(virDomainDefPtr def, _("Setting wwn is not supported for lun device")); goto error; } + if (disk->vendor || disk->product) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Setting vendor or product is not supported for lun device")); + goto error; + } } switch (disk->bus) { @@ -2504,6 +2516,17 @@ qemuBuildDriveDevStr(virDomainDefPtr def, goto error; } + /* Properties wwn, vendor and product were introduced in the + * same QEMU release (1.2.0). + */ + if ((disk->vendor || disk->product) && + !qemuCapsGet(caps, QEMU_CAPS_SCSI_DISK_WWN)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Setting vendor or product for scsi disk is not " + "supported by this QEMU")); + goto error; + } + controllerModel = virDomainDiskFindControllerModel(def, disk, VIR_DOMAIN_CONTROLLER_TYPE_SCSI); @@ -2649,6 +2672,12 @@ qemuBuildDriveDevStr(virDomainDefPtr def, if (disk->wwn) virBufferAsprintf(&opt, ",wwn=%s", disk->wwn); + if (disk->vendor) + virBufferAsprintf(&opt, ",vendor=%s", disk->vendor); + + if (disk->product) + virBufferAsprintf(&opt, ",product=%s", disk->product); + if (virBufferError(&opt)) { virReportOOMError(); goto error; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args new file mode 100644 index 0000000..4aefb7f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args @@ -0,0 +1,13 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 \ +-device lsi,id=scsi1,bus=pci.0,addr=0x4 \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-1-0 \ +-device scsi-cd,bus=scsi0.0,channel=0,scsi-id=1,lun=0,drive=drive-scsi0-0-1-0,\ +id=scsi0-0-1-0,vendor=SEAGATE,product=ST3146707LC \ +-drive file=/dev/HostVG/QEMUGuest2,if=none,id=drive-scsi0-0-0-0 \ +-device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,\ +id=scsi0-0-0-0,vendor=SEAGATE,product=ST3567807GD \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.xml new file mode 100644 index 0000000..4918e37 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='cdrom'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='1' unit='0'/> + <vendor>SEAGATE</vendor> + <product>ST3146707LC</product> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='sdb' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + <vendor>SEAGATE</vendor> + <product>ST3567807GD</product> + </disk> + <controller type='usb' index='0'/> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='scsi' index='1' model='lsilogic'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 20b0b35..39a7e3f 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -499,6 +499,10 @@ mymain(void) QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_SCSI_CD, QEMU_CAPS_SCSI_LSI, QEMU_CAPS_VIRTIO_SCSI_PCI, QEMU_CAPS_SCSI_DISK_WWN); + DO_TEST("disk-scsi-disk-vpd", + QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_SCSI_CD, QEMU_CAPS_SCSI_LSI, QEMU_CAPS_VIRTIO_SCSI_PCI, + QEMU_CAPS_SCSI_DISK_WWN); DO_TEST("disk-scsi-vscsi", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); DO_TEST("disk-scsi-virtio-scsi", -- 1.7.7.6
On 11/05/2012 08:04 AM, Osier Yang wrote:
QEMU supports to set vendor and product strings for disk since 1.2.0 (only scsi-disk, scsi-hd, scsi-cd support it), this patch exposes it with new XML elements <vendor> and <product> of disk device. --- docs/formatdomain.html.in | 10 +++++ docs/schemas/domaincommon.rng | 10 +++++ src/conf/domain_conf.c | 30 ++++++++++++++++ src/conf/domain_conf.h | 2 + src/qemu/qemu_command.c | 29 ++++++++++++++++ .../qemuxml2argv-disk-scsi-disk-vpd.args | 13 +++++++ .../qemuxml2argv-disk-scsi-disk-vpd.xml | 36 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++ 8 files changed, 134 insertions(+), 0 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c8da33d..cc9e871 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1657,6 +1657,16 @@ of 16 hexadecimal digits. <span class='since'>Since 0.10.1</span> </dd> + <dt><code>vendor</code></dt> + <dd>If present, this element specifies the vendor of a virtual hard + disk or CD-ROM device. It's a not more than 8 bytes alphanumeric string.
Last sentence doesn't make sense, I suggest changing it to either: "It can't be longer than 8 alphanumeric characters." or simply "Maximum 8 alphanumeric characters."
+ <span class='since'>Since 1.0.1</span> + </dd> + <dt><code>product</code></dt> + <dd>If present, this element specifies the product of a virtual hard + disk or CD-ROM device. It's a not more than 16 bytes alphanumeric string.
dtto
+ <span class='since'>Since 1.0.1</span> + </dd> <dt><code>host</code></dt> <dd>The <code>host</code> element has two attributes "name" and "port", which specify the hostname and the port number. The meaning of this diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 2beb035..ed7d1d0 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -905,6 +905,16 @@ <ref name="wwn"/> </element> </optional> + <optional> + <element name="vendor"> + <text/> + </element> + </optional> + <optional> + <element name="product"> + <text/> + </element> + </optional>
This is little bit different than the other specifications around in the code and could be made better, but looking at the schema I've found more inconsistencies, so it's ok for now. I'll send a cleanup patch later for these and the others as well.
</interleave> </define> <define name="snapshot"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0575fcd..db6608e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -979,6 +979,8 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def->mirror); VIR_FREE(def->auth.username); VIR_FREE(def->wwn); + VIR_FREE(def->vendor); + VIR_FREE(def->product); if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE) VIR_FREE(def->auth.secret.usage); virStorageEncryptionFree(def->encryption); @@ -3498,6 +3500,8 @@ cleanup: goto cleanup; }
+#define VENDOR_LEN 8 +#define PRODUCT_LEN 16
/* Parse the XML definition for a disk * @param node XML nodeset to parse for disk definition @@ -3550,6 +3554,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *logical_block_size = NULL; char *physical_block_size = NULL; char *wwn = NULL; + char *vendor = NULL; + char *product = NULL;
if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -3888,6 +3894,24 @@ virDomainDiskDefParseXML(virCapsPtr caps,
if (!virValidateWWN(wwn)) goto error; + } else if (!vendor && + xmlStrEqual(cur->name, BAD_CAST "vendor")) { + vendor = (char *)xmlNodeGetContent(cur); + + if (strlen(vendor) > VENDOR_LEN) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("disk vendor is more than 8 bytes string"));
This should be "disk vendor name is more than 8 characters long" or ".. is longer than 8 characters", also there is no check these are alphanumeric although it is mentioned in the documentation. I believe we can use something similar to virValidateWWN().
+ goto error; + } + } else if (!product && + xmlStrEqual(cur->name, BAD_CAST "product")) { + product = (char *)xmlNodeGetContent(cur); + + if (strlen(vendor) > PRODUCT_LEN) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("disk product is more than 16 bytes string"));
dtto
+ goto error; + } } else if (xmlStrEqual(cur->name, BAD_CAST "boot")) { /* boot is parsed as part of virDomainDeviceInfoParseXML */ } @@ -4184,6 +4208,10 @@ virDomainDiskDefParseXML(virCapsPtr caps, serial = NULL; def->wwn = wwn; wwn = NULL; + def->vendor = vendor; + vendor = NULL; + def->product = product; + product = NULL;
if (driverType) { def->format = virStorageFileFormatTypeFromString(driverType); @@ -4257,6 +4285,8 @@ cleanup: VIR_FREE(logical_block_size); VIR_FREE(physical_block_size); VIR_FREE(wwn); + VIR_FREE(vendor); + VIR_FREE(product);
ctxt->node = save_ctxt; return def; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6539281..c7c1ca6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -591,6 +591,8 @@ struct _virDomainDiskDef {
char *serial; char *wwn; + char *vendor; + char *product; int cachemode; int error_policy; /* enum virDomainDiskErrorPolicy */ int rerror_policy; /* enum virDomainDiskErrorPolicy */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 389c480..b0b81f3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2428,6 +2428,13 @@ qemuBuildDriveDevStr(virDomainDefPtr def, } }
+ if ((disk->vendor || disk->product) && + disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Only scsi disk support vendor and product"));
Either s/disk/disks/ or s/support/supports/, I'd rather see the former.
+ goto error; + } + if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { /* make sure that both the bus and the qemu binary support * type='lun' (SG_IO). @@ -2455,6 +2462,11 @@ qemuBuildDriveDevStr(virDomainDefPtr def, _("Setting wwn is not supported for lun device")); goto error; } + if (disk->vendor || disk->product) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Setting vendor or product is not supported for lun device")); + goto error; + } }
switch (disk->bus) { @@ -2504,6 +2516,17 @@ qemuBuildDriveDevStr(virDomainDefPtr def, goto error; }
+ /* Properties wwn, vendor and product were introduced in the + * same QEMU release (1.2.0). + */ + if ((disk->vendor || disk->product) && + !qemuCapsGet(caps, QEMU_CAPS_SCSI_DISK_WWN)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Setting vendor or product for scsi disk is not " + "supported by this QEMU")); + goto error; + } + controllerModel = virDomainDiskFindControllerModel(def, disk, VIR_DOMAIN_CONTROLLER_TYPE_SCSI); @@ -2649,6 +2672,12 @@ qemuBuildDriveDevStr(virDomainDefPtr def, if (disk->wwn) virBufferAsprintf(&opt, ",wwn=%s", disk->wwn);
+ if (disk->vendor) + virBufferAsprintf(&opt, ",vendor=%s", disk->vendor); + + if (disk->product) + virBufferAsprintf(&opt, ",product=%s", disk->product); + if (virBufferError(&opt)) { virReportOOMError(); goto error; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args new file mode 100644 index 0000000..4aefb7f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args @@ -0,0 +1,13 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 \ +-device lsi,id=scsi1,bus=pci.0,addr=0x4 \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-1-0 \ +-device scsi-cd,bus=scsi0.0,channel=0,scsi-id=1,lun=0,drive=drive-scsi0-0-1-0,\ +id=scsi0-0-1-0,vendor=SEAGATE,product=ST3146707LC \ +-drive file=/dev/HostVG/QEMUGuest2,if=none,id=drive-scsi0-0-0-0 \ +-device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,\ +id=scsi0-0-0-0,vendor=SEAGATE,product=ST3567807GD \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.xml new file mode 100644 index 0000000..4918e37 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='cdrom'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='1' unit='0'/> + <vendor>SEAGATE</vendor> + <product>ST3146707LC</product> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='sdb' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + <vendor>SEAGATE</vendor> + <product>ST3567807GD</product> + </disk> + <controller type='usb' index='0'/> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='scsi' index='1' model='lsilogic'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 20b0b35..39a7e3f 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -499,6 +499,10 @@ mymain(void) QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_SCSI_CD, QEMU_CAPS_SCSI_LSI, QEMU_CAPS_VIRTIO_SCSI_PCI, QEMU_CAPS_SCSI_DISK_WWN); + DO_TEST("disk-scsi-disk-vpd", + QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_SCSI_CD, QEMU_CAPS_SCSI_LSI, QEMU_CAPS_VIRTIO_SCSI_PCI, + QEMU_CAPS_SCSI_DISK_WWN); DO_TEST("disk-scsi-vscsi", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); DO_TEST("disk-scsi-virtio-scsi",
Good to have tests, even better would be to have some failure tests as well. Martin
On 2012年11月05日 21:34, Martin Kletzander wrote:
On 11/05/2012 08:04 AM, Osier Yang wrote:
QEMU supports to set vendor and product strings for disk since 1.2.0 (only scsi-disk, scsi-hd, scsi-cd support it), this patch exposes it with new XML elements<vendor> and<product> of disk device. --- docs/formatdomain.html.in | 10 +++++ docs/schemas/domaincommon.rng | 10 +++++ src/conf/domain_conf.c | 30 ++++++++++++++++ src/conf/domain_conf.h | 2 + src/qemu/qemu_command.c | 29 ++++++++++++++++ .../qemuxml2argv-disk-scsi-disk-vpd.args | 13 +++++++ .../qemuxml2argv-disk-scsi-disk-vpd.xml | 36 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++ 8 files changed, 134 insertions(+), 0 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c8da33d..cc9e871 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1657,6 +1657,16 @@ of 16 hexadecimal digits. <span class='since'>Since 0.10.1</span> </dd> +<dt><code>vendor</code></dt> +<dd>If present, this element specifies the vendor of a virtual hard + disk or CD-ROM device. It's a not more than 8 bytes alphanumeric string.
Last sentence doesn't make sense, I suggest changing it to either: "It can't be longer than 8 alphanumeric characters." or simply "Maximum 8 alphanumeric characters."
Okay, honestly, I wasn't comfortable with the sentence, but can't get a better one at that time, :-) I will change your advise a bit: It must not be longer than 8 alphanumeric characters.
+<span class='since'>Since 1.0.1</span> +</dd> +<dt><code>product</code></dt> +<dd>If present, this element specifies the product of a virtual hard + disk or CD-ROM device. It's a not more than 16 bytes alphanumeric string.
dtto
+<span class='since'>Since 1.0.1</span> +</dd> <dt><code>host</code></dt> <dd>The<code>host</code> element has two attributes "name" and "port", which specify the hostname and the port number. The meaning of this diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 2beb035..ed7d1d0 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -905,6 +905,16 @@ <ref name="wwn"/> </element> </optional> +<optional> +<element name="vendor"> +<text/> +</element> +</optional> +<optional> +<element name="product"> +<text/> +</element> +</optional>
This is little bit different than the other specifications around in the code and could be made better, but looking at the schema I've found more inconsistencies, so it's ok for now. I'll send a cleanup patch later for these and the others as well.
</interleave> </define> <define name="snapshot"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0575fcd..db6608e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -979,6 +979,8 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def->mirror); VIR_FREE(def->auth.username); VIR_FREE(def->wwn); + VIR_FREE(def->vendor); + VIR_FREE(def->product); if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE) VIR_FREE(def->auth.secret.usage); virStorageEncryptionFree(def->encryption); @@ -3498,6 +3500,8 @@ cleanup: goto cleanup; }
+#define VENDOR_LEN 8 +#define PRODUCT_LEN 16
/* Parse the XML definition for a disk * @param node XML nodeset to parse for disk definition @@ -3550,6 +3554,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *logical_block_size = NULL; char *physical_block_size = NULL; char *wwn = NULL; + char *vendor = NULL; + char *product = NULL;
if (VIR_ALLOC(def)< 0) { virReportOOMError(); @@ -3888,6 +3894,24 @@ virDomainDiskDefParseXML(virCapsPtr caps,
if (!virValidateWWN(wwn)) goto error; + } else if (!vendor&& + xmlStrEqual(cur->name, BAD_CAST "vendor")) { + vendor = (char *)xmlNodeGetContent(cur); + + if (strlen(vendor)> VENDOR_LEN) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("disk vendor is more than 8 bytes string"));
This should be "disk vendor name is more than 8 characters long" or ".. is longer than 8 characters", also there is no check these are alphanumeric although it is mentioned in the documentation. I believe we can use something similar to virValidateWWN().
Okay, since I already tried to validate the strings, further checking makes sense. Sorry for the late response, will send v2 soon. Regards, Osier
On Wed, Nov 07, 2012 at 11:24:43PM +0800, Osier Yang wrote:
On 2012年11月05日 21:34, Martin Kletzander wrote:
On 11/05/2012 08:04 AM, Osier Yang wrote:
QEMU supports to set vendor and product strings for disk since 1.2.0 (only scsi-disk, scsi-hd, scsi-cd support it), this patch exposes it with new XML elements<vendor> and<product> of disk device. --- docs/formatdomain.html.in | 10 +++++ docs/schemas/domaincommon.rng | 10 +++++ src/conf/domain_conf.c | 30 ++++++++++++++++ src/conf/domain_conf.h | 2 + src/qemu/qemu_command.c | 29 ++++++++++++++++ .../qemuxml2argv-disk-scsi-disk-vpd.args | 13 +++++++ .../qemuxml2argv-disk-scsi-disk-vpd.xml | 36 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++ 8 files changed, 134 insertions(+), 0 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c8da33d..cc9e871 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1657,6 +1657,16 @@ of 16 hexadecimal digits. <span class='since'>Since 0.10.1</span> </dd> +<dt><code>vendor</code></dt> +<dd>If present, this element specifies the vendor of a virtual hard + disk or CD-ROM device. It's a not more than 8 bytes alphanumeric string.
Last sentence doesn't make sense, I suggest changing it to either: "It can't be longer than 8 alphanumeric characters." or simply "Maximum 8 alphanumeric characters."
Okay, honestly, I wasn't comfortable with the sentence, but can't get a better one at that time, :-) I will change your advise a bit:
It must not be longer than 8 alphanumeric characters.
Not to be pedantic, but what happens if you hand it doublebyte characters?
+<span class='since'>Since 1.0.1</span> +</dd> +<dt><code>product</code></dt> +<dd>If present, this element specifies the product of a virtual hard + disk or CD-ROM device. It's a not more than 16 bytes alphanumeric string.
dtto
+<span class='since'>Since 1.0.1</span> +</dd> <dt><code>host</code></dt> <dd>The<code>host</code> element has two attributes "name" and "port", which specify the hostname and the port number. The meaning of this diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 2beb035..ed7d1d0 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -905,6 +905,16 @@ <ref name="wwn"/> </element> </optional> +<optional> +<element name="vendor"> +<text/> +</element> +</optional> +<optional> +<element name="product"> +<text/> +</element> +</optional>
This is little bit different than the other specifications around in the code and could be made better, but looking at the schema I've found more inconsistencies, so it's ok for now. I'll send a cleanup patch later for these and the others as well.
</interleave> </define> <define name="snapshot"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0575fcd..db6608e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -979,6 +979,8 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def->mirror); VIR_FREE(def->auth.username); VIR_FREE(def->wwn); + VIR_FREE(def->vendor); + VIR_FREE(def->product); if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE) VIR_FREE(def->auth.secret.usage); virStorageEncryptionFree(def->encryption); @@ -3498,6 +3500,8 @@ cleanup: goto cleanup; }
+#define VENDOR_LEN 8 +#define PRODUCT_LEN 16
/* Parse the XML definition for a disk * @param node XML nodeset to parse for disk definition @@ -3550,6 +3554,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *logical_block_size = NULL; char *physical_block_size = NULL; char *wwn = NULL; + char *vendor = NULL; + char *product = NULL;
if (VIR_ALLOC(def)< 0) { virReportOOMError(); @@ -3888,6 +3894,24 @@ virDomainDiskDefParseXML(virCapsPtr caps,
if (!virValidateWWN(wwn)) goto error; + } else if (!vendor&& + xmlStrEqual(cur->name, BAD_CAST "vendor")) { + vendor = (char *)xmlNodeGetContent(cur); + + if (strlen(vendor)> VENDOR_LEN) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("disk vendor is more than 8 bytes string"));
This should be "disk vendor name is more than 8 characters long" or ".. is longer than 8 characters", also there is no check these are alphanumeric although it is mentioned in the documentation. I believe we can use something similar to virValidateWWN().
Okay, since I already tried to validate the strings, further checking makes sense.
Sorry for the late response, will send v2 soon.
Regards, Osier
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 2012年11月08日 05:04, Dave Allan wrote:
On Wed, Nov 07, 2012 at 11:24:43PM +0800, Osier Yang wrote:
On 2012年11月05日 21:34, Martin Kletzander wrote:
On 11/05/2012 08:04 AM, Osier Yang wrote:
QEMU supports to set vendor and product strings for disk since 1.2.0 (only scsi-disk, scsi-hd, scsi-cd support it), this patch exposes it with new XML elements<vendor> and<product> of disk device. --- docs/formatdomain.html.in | 10 +++++ docs/schemas/domaincommon.rng | 10 +++++ src/conf/domain_conf.c | 30 ++++++++++++++++ src/conf/domain_conf.h | 2 + src/qemu/qemu_command.c | 29 ++++++++++++++++ .../qemuxml2argv-disk-scsi-disk-vpd.args | 13 +++++++ .../qemuxml2argv-disk-scsi-disk-vpd.xml | 36 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++ 8 files changed, 134 insertions(+), 0 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c8da33d..cc9e871 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1657,6 +1657,16 @@ of 16 hexadecimal digits. <span class='since'>Since 0.10.1</span> </dd> +<dt><code>vendor</code></dt> +<dd>If present, this element specifies the vendor of a virtual hard + disk or CD-ROM device. It's a not more than 8 bytes alphanumeric string.
Last sentence doesn't make sense, I suggest changing it to either: "It can't be longer than 8 alphanumeric characters." or simply "Maximum 8 alphanumeric characters."
Okay, honestly, I wasn't comfortable with the sentence, but can't get a better one at that time, :-) I will change your advise a bit:
It must not be longer than 8 alphanumeric characters.
Not to be pedantic, but what happens if you hand it doublebyte characters?
Ah, good question, though QEMU will truncate the string to 8 bytes anyway, should we do some translation in libvirt? the problem is how to map the doublebytes vendors/product to single bytes ones, is there some public specification available? /me starts to think if it's snake leg (or breaking fly on the wheel) to do the checking if it's too heavy. Regards, Osier
On Thu, Nov 08, 2012 at 09:28:06AM +0800, Osier Yang wrote:
On 2012年11月08日 05:04, Dave Allan wrote:
On Wed, Nov 07, 2012 at 11:24:43PM +0800, Osier Yang wrote:
On 2012年11月05日 21:34, Martin Kletzander wrote:
On 11/05/2012 08:04 AM, Osier Yang wrote:
QEMU supports to set vendor and product strings for disk since 1.2.0 (only scsi-disk, scsi-hd, scsi-cd support it), this patch exposes it with new XML elements<vendor> and<product> of disk device. --- docs/formatdomain.html.in | 10 +++++ docs/schemas/domaincommon.rng | 10 +++++ src/conf/domain_conf.c | 30 ++++++++++++++++ src/conf/domain_conf.h | 2 + src/qemu/qemu_command.c | 29 ++++++++++++++++ .../qemuxml2argv-disk-scsi-disk-vpd.args | 13 +++++++ .../qemuxml2argv-disk-scsi-disk-vpd.xml | 36 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++ 8 files changed, 134 insertions(+), 0 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c8da33d..cc9e871 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1657,6 +1657,16 @@ of 16 hexadecimal digits. <span class='since'>Since 0.10.1</span> </dd> +<dt><code>vendor</code></dt> +<dd>If present, this element specifies the vendor of a virtual hard + disk or CD-ROM device. It's a not more than 8 bytes alphanumeric string.
Last sentence doesn't make sense, I suggest changing it to either: "It can't be longer than 8 alphanumeric characters." or simply "Maximum 8 alphanumeric characters."
Okay, honestly, I wasn't comfortable with the sentence, but can't get a better one at that time, :-) I will change your advise a bit:
It must not be longer than 8 alphanumeric characters.
Not to be pedantic, but what happens if you hand it doublebyte characters?
Ah, good question, though QEMU will truncate the string to 8 bytes anyway, should we do some translation in libvirt? the problem is how to map the doublebytes vendors/product to single bytes ones, is there some public specification available? /me starts to think if it's snake leg (or breaking fly on the wheel) to do the checking if it's too heavy.
Mostly I was curious about how the code handled doublebyte characters, but now that I think about it, the SCSI spec mandates 8 bytes of ASCII[1], but it sounds like qemu is already enforcing that, so I think it's fine just to let it be freeform and note the spec's requirement in the documentation. (I had to google "putting legs on a snake", and yes, we should avoid that.) Dave [1] http://en.wikipedia.org/wiki/SCSI_Inquiry_Command
On 11/08/2012 02:52 AM, Dave Allan wrote:
On Thu, Nov 08, 2012 at 09:28:06AM +0800, Osier Yang wrote:
On 2012年11月08日 05:04, Dave Allan wrote:
On Wed, Nov 07, 2012 at 11:24:43PM +0800, Osier Yang wrote:
On 2012年11月05日 21:34, Martin Kletzander wrote:
On 11/05/2012 08:04 AM, Osier Yang wrote:
QEMU supports to set vendor and product strings for disk since 1.2.0 (only scsi-disk, scsi-hd, scsi-cd support it), this patch exposes it with new XML elements<vendor> and<product> of disk device. --- docs/formatdomain.html.in | 10 +++++ docs/schemas/domaincommon.rng | 10 +++++ src/conf/domain_conf.c | 30 ++++++++++++++++ src/conf/domain_conf.h | 2 + src/qemu/qemu_command.c | 29 ++++++++++++++++ .../qemuxml2argv-disk-scsi-disk-vpd.args | 13 +++++++ .../qemuxml2argv-disk-scsi-disk-vpd.xml | 36 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++ 8 files changed, 134 insertions(+), 0 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c8da33d..cc9e871 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1657,6 +1657,16 @@ of 16 hexadecimal digits. <span class='since'>Since 0.10.1</span> </dd> +<dt><code>vendor</code></dt> +<dd>If present, this element specifies the vendor of a virtual hard + disk or CD-ROM device. It's a not more than 8 bytes alphanumeric string.
Last sentence doesn't make sense, I suggest changing it to either: "It can't be longer than 8 alphanumeric characters." or simply "Maximum 8 alphanumeric characters."
Okay, honestly, I wasn't comfortable with the sentence, but can't get a better one at that time, :-) I will change your advise a bit:
It must not be longer than 8 alphanumeric characters.
Not to be pedantic, but what happens if you hand it doublebyte characters?
Ah, good question, though QEMU will truncate the string to 8 bytes anyway, should we do some translation in libvirt? the problem is how to map the doublebytes vendors/product to single bytes ones, is there some public specification available? /me starts to think if it's snake leg (or breaking fly on the wheel) to do the checking if it's too heavy.
Mostly I was curious about how the code handled doublebyte characters, but now that I think about it, the SCSI spec mandates 8 bytes of ASCII[1], but it sounds like qemu is already enforcing that, so I think it's fine just to let it be freeform and note the spec's requirement in the documentation.
I'd say if QEMU starts (and truncates or whatever) with invalid vendor name, there's no problem for us to leave the responsibility on the user, but OTOH when QEMU won't like what it's doing and will eventually fix/change that, our machines may not start. So if we know the limitation (and it is very easy one), why don't we just check (and mention it in the docs) that we accept maximum 8 (16) alphanumeric characters, checking just [A-Za-z0-9] instead of doing some 'isalnum()' magic. Check is easy, QEMU will be always satisfied with the option, I see it as a win-win. Martin
On 2012年11月08日 16:19, Martin Kletzander wrote:
On 11/08/2012 02:52 AM, Dave Allan wrote:
On Thu, Nov 08, 2012 at 09:28:06AM +0800, Osier Yang wrote:
On 2012年11月08日 05:04, Dave Allan wrote:
On Wed, Nov 07, 2012 at 11:24:43PM +0800, Osier Yang wrote:
On 2012年11月05日 21:34, Martin Kletzander wrote:
On 11/05/2012 08:04 AM, Osier Yang wrote: > QEMU supports to set vendor and product strings for disk since > 1.2.0 (only scsi-disk, scsi-hd, scsi-cd support it), this patch > exposes it with new XML elements<vendor> and<product> of disk > device. > --- > docs/formatdomain.html.in | 10 +++++ > docs/schemas/domaincommon.rng | 10 +++++ > src/conf/domain_conf.c | 30 ++++++++++++++++ > src/conf/domain_conf.h | 2 + > src/qemu/qemu_command.c | 29 ++++++++++++++++ > .../qemuxml2argv-disk-scsi-disk-vpd.args | 13 +++++++ > .../qemuxml2argv-disk-scsi-disk-vpd.xml | 36 ++++++++++++++++++++ > tests/qemuxml2argvtest.c | 4 ++ > 8 files changed, 134 insertions(+), 0 deletions(-) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.xml > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index c8da33d..cc9e871 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -1657,6 +1657,16 @@ > of 16 hexadecimal digits. > <span class='since'>Since 0.10.1</span> > </dd> > +<dt><code>vendor</code></dt> > +<dd>If present, this element specifies the vendor of a virtual hard > + disk or CD-ROM device. It's a not more than 8 bytes alphanumeric string.
Last sentence doesn't make sense, I suggest changing it to either: "It can't be longer than 8 alphanumeric characters." or simply "Maximum 8 alphanumeric characters."
Okay, honestly, I wasn't comfortable with the sentence, but can't get a better one at that time, :-) I will change your advise a bit:
It must not be longer than 8 alphanumeric characters.
Not to be pedantic, but what happens if you hand it doublebyte characters?
Ah, good question, though QEMU will truncate the string to 8 bytes anyway, should we do some translation in libvirt? the problem is how to map the doublebytes vendors/product to single bytes ones, is there some public specification available? /me starts to think if it's snake leg (or breaking fly on the wheel) to do the checking if it's too heavy.
Mostly I was curious about how the code handled doublebyte characters, but now that I think about it, the SCSI spec mandates 8 bytes of ASCII[1], but it sounds like qemu is already enforcing that, so I think it's fine just to let it be freeform and note the spec's requirement in the documentation.
I'd say if QEMU starts (and truncates or whatever) with invalid vendor name, there's no problem for us to leave the responsibility on the user, but OTOH when QEMU won't like what it's doing and will eventually fix/change that, our machines may not start. So if we know the limitation (and it is very easy one), why don't we just check (and mention it in the docs) that we accept maximum 8 (16) alphanumeric characters, checking just [A-Za-z0-9] instead of doing some 'isalnum()' magic. Check is easy, QEMU will be always satisfied with the option, I see it as a win-win.
Not really, QEMU won't be happy anyway if it really changes the limits. But I agree with adding the checking, as per the SCSI spec mandates 8/16 bytes. We don't need to care about the doublebytes then. And a good reason for the hecking is error out earlier is always good than QEMU process fails to start. And it's not likely QEMU will change the limits to violate the spec. Regards, Osier
participants (3)
-
Dave Allan -
Martin Kletzander -
Osier Yang