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