[libvirt] [PATCH v5] qemu: Allow the user to specify vendor and product for disk

Fri Dec 7 08:56:18 UTC 2012

On 2012年12月07日 01:57, Osier Yang wrote:
> On 2012年12月06日 22:04, Michal Privoznik wrote:
>> 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.
>
> Personally I'd like keep it in separate line, but okay, fit into
> previous line is also fine, it's trivial.
>
>>
>>> +</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.
>
> Okay.
>
>>
>>> +<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?
>
> Do you mean the doc? if so there are declarations in
> formatdomain.html.in, or you mean some spec for the
> limitations? there is specification for that, but I
> can't find it right now.
>
>>
>>> </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.
>>
>
> Yes, most of these vars are just redundant, could be cleaned up as a
> later patch.
>
>>>
>>> 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.
>
> That's the magic thing of virBufferEscapeString, it doesn't print
> anything if the passed string is NULL. Perhaps we should rename
> the function, it causes confusoin to many guys as far as I see.
>
>>
>>> 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
>
> Okay.
>
>>
>>> + 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.
>
> Thanks for the reviewing! I will push it tomorrow.
>

Now pushed with the long line and the missed half-sentence doc fixed.