On 06.12.2012 11:23, Osier Yang wrote:
> QEMU supports setting 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.
>
> v4 - v5:
> * Just rebasing
>
> v3 - v4:
>
> * Per Paolo's feedback, allows all printable chars.
> ---
> I think it's pretty straightforward to ACK now, any one can review
> it?
>
> ---
> docs/formatdomain.html.in | 11 +++++
> docs/schemas/domaincommon.rng | 14 ++++++
> src/conf/domain_conf.c | 44 ++++++++++++++++++++
> src/conf/domain_conf.h | 2 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_command.c | 29 +++++++++++++
> src/util/util.c | 12 +++++
> src/util/util.h | 1 +
> ...qemuxml2argv-disk-scsi-disk-vpd-build-error.xml | 35 ++++++++++++++++
> .../qemuxml2argv-disk-scsi-disk-vpd.args | 13 ++++++
> .../qemuxml2argv-disk-scsi-disk-vpd.xml | 38 +++++++++++++++++
> tests/qemuxml2argvtest.c | 8 ++++
> tests/qemuxml2xmltest.c | 2 +
> 13 files changed, 210 insertions(+), 0 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd-build-error.xml
> 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 0574e68..30fb021 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1657,6 +1657,17 @@
> 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 must not be longer than 8 printable
> + characters.
> + <span class='since'>Since 1.0.1</span>
This <span/> would fit into previous line.
> + </dd>
> + <dt><code>product</code></dt>
> + <dd>If present, this element specifies the product of a virtual hard
> + disk or CD-ROM device. It must not be longer than 16 printable
* ... end of the sentence is missing. Probably deleted by mistake.
> + <span class='since'>Since 1.0.1</span>
> + </dd>
> <dt><code>host</code></dt>
> <dd>The <code>host</code> element supports 4 attributes, viz. "name",
> "port", "transport" and "socket", which specify the hostname, the port
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 0e85739..14344e2 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -905,6 +905,20 @@
> <ref name="wwn"/>
> </element>
> </optional>
> + <optional>
> + <element name="vendor">
> + <data type="string">
> + <param name="pattern">[x20-x7E]{0,8}</param>
> + </data>
> + </element>
> + </optional>
> + <optional>
> + <element name="product">
> + <data type="string">
> + <param name="pattern">[x20-x7E]{0,16}</param>
> + </data>
> + </element>
> + </optional>
Just curious - is this limitation defined somewhere?
> </interleave>
> </define>
> <define name="snapshot">
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 4ffb39d..6aa5f79 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -985,6 +985,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);
> @@ -3505,6 +3507,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
> @@ -3558,6 +3562,8 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> char *logical_block_size = NULL;
> char *physical_block_size = NULL;
> char *wwn = NULL;
> + char *vendor = NULL;
> + char *product = NULL;
Why do we need these variables? I guess def->{vendor,product} cab be
used directly here. But on the other hand - you're just keeping the
style used within the function.
>
> if (VIR_ALLOC(def) < 0) {
> virReportOOMError();
> @@ -3926,6 +3932,36 @@ 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 characters"));
> + goto error;
> + }
> +
> + if (!virStrIsPrint(vendor)) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("disk vendor is not printable 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 characters"));
> + goto error;
> + }
> +
> + if (!virStrIsPrint(product)) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("disk product is not printable string"));
> + goto error;
> + }
> } else if (xmlStrEqual(cur->name, BAD_CAST "boot")) {
> /* boot is parsed as part of virDomainDeviceInfoParseXML */
> }
> @@ -4222,6 +4258,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);
> @@ -4296,6 +4336,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;
> @@ -12144,6 +12186,8 @@ virDomainDiskDefFormat(virBufferPtr buf,
> virBufferAddLit(buf, " <transient/>\n");
> virBufferEscapeString(buf, " <serial>%s</serial>\n", def->serial);
> virBufferEscapeString(buf, " <wwn>%s</wwn>\n", def->wwn);
> + virBufferEscapeString(buf, " <vendor>%s</vendor>\n", def->vendor);
> + virBufferEscapeString(buf, " <product>%s</product>\n", def->product);
* I believe this should be conditional: print vendor only if there's
def->vendor. Same applies to def->product.
> if (def->encryption) {
> virBufferAdjustIndent(buf, 6);
> if (virStorageEncryptionFormat(buf, def->encryption) < 0)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 4ab15e9..7ad5377 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -602,6 +602,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/libvirt_private.syms b/src/libvirt_private.syms
> index bc01fe5..499ab3b 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1298,6 +1298,7 @@ virSetUIDGID;
> virSkipSpaces;
> virSkipSpacesAndBackslash;
> virSkipSpacesBackwards;
> +virStrIsPrint;
> virStrToDouble;
> virStrToLong_i;
> virStrToLong_l;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 1805625..826086c 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2576,6 +2576,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 supports 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).
> @@ -2603,6 +2610,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"));
long line
> + goto error;
> + }
> }
>
> switch (disk->bus) {
> @@ -2652,6 +2664,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);
> @@ -2797,6 +2820,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/src/util/util.c b/src/util/util.c
> index 2fd0f2c..95b0a06 100644
> --- a/src/util/util.c
> +++ b/src/util/util.c
> @@ -3113,3 +3113,15 @@ virValidateWWN(const char *wwn) {
>
> return true;
> }
> +
> +bool
> +virStrIsPrint(const char *str)
> +{
> + int i;
> +
> + for (i = 0; str[i]; i++)
> + if (!c_isprint(str[i]))
> + return false;
> +
> + return true;
> +}
> diff --git a/src/util/util.h b/src/util/util.h
> index 4316ab1..6d5dd03 100644
> --- a/src/util/util.h
> +++ b/src/util/util.h
> @@ -280,4 +280,5 @@ bool virIsDevMapperDevice(const char *dev_name) ATTRIBUTE_NONNULL(1);
>
> bool virValidateWWN(const char *wwn);
>
> +bool virStrIsPrint(const char *str);
> #endif /* __VIR_UTIL_H__ */
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd-build-error.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd-build-error.xml
> new file mode 100644
> index 0000000..ca68275
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd-build-error.xml
> @@ -0,0 +1,35 @@
> +<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='virtio'/>
> + <vendor>SEAGATE</vendor>
> + <product>ST3146707LC</product>
> + </disk>
> + <disk type='block' device='disk'>
> + <source dev='/dev/HostVG/QEMUGuest2'/>
> + <target dev='sdb' bus='scsi'/>
> + <vendor>SEAGATE</vendor>
> + <product>ST3567807GD</product>
> + <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> + </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/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args
> new file mode 100644
> index 0000000..f5c1999
> --- /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-0-0 \
> +-device scsi-cd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,\
> +id=scsi0-0-0-0,vendor=SEAGATE,product=ST3146707LC \
> +-drive file=/dev/HostVG/QEMUGuest2,if=none,id=drive-scsi1-0-0 \
> +-device scsi-hd,bus=scsi1.0,scsi-id=0,drive=drive-scsi1-0-0,\
> +id=scsi1-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..96786e3
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.xml
> @@ -0,0 +1,38 @@
> +<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'/>
> + <readonly/>
> + <vendor>SEAGATE</vendor>
> + <product>ST3146707LC</product>
> + <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> + </disk>
> + <disk type='block' device='disk'>
> + <source dev='/dev/HostVG/QEMUGuest2'/>
> + <target dev='sdb' bus='scsi'/>
> + <readonly/>
> + <vendor>SEAGATE</vendor>
> + <product>ST3567807GD</product>
> + <address type='drive' controller='1' bus='0' target='0' unit='0'/>
> + </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 5822cae..bb233ed 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -507,6 +507,14 @@ 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_FAILURE("disk-scsi-disk-vpd-build-error",
> + 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",
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 3d8176c..c2d3dd3 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -239,6 +239,8 @@ mymain(void)
> DO_TEST("seclabel-none");
> DO_TEST("numad-static-vcpu-no-numatune");
>
> + DO_TEST("disk-scsi-disk-vpd");
> +
> /* These tests generate different XML */
> DO_TEST_DIFFERENT("balloon-device-auto");
> DO_TEST_DIFFERENT("channel-virtio-auto");
>
ACK if all issues marked with * (ideally all) fixed.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list