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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list