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

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.