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

Osier Yang jyang at redhat.com
Thu Dec 6 17:57:27 UTC 2012


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.

Regards,
Osier




More information about the libvir-list mailing list