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

On 2012年11月05日 21:34, Martin Kletzander wrote: >On 11/05/2012 08:04 AM, Osier Yang wrote: >>QEMU supports to set 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. >>--- >> docs/formatdomain.html.in | 10 +++++ >> docs/schemas/domaincommon.rng | 10 +++++ >> src/conf/domain_conf.c | 30 ++++++++++++++++ >> src/conf/domain_conf.h | 2 + >> src/qemu/qemu_command.c | 29 ++++++++++++++++ >> .../qemuxml2argv-disk-scsi-disk-vpd.args | 13 +++++++ >> .../qemuxml2argv-disk-scsi-disk-vpd.xml | 36 ++++++++++++++++++++ >> tests/qemuxml2argvtest.c | 4 ++ >> 8 files changed, 134 insertions(+), 0 deletions(-) >> 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 c8da33d..cc9e871 100644 >>--- a/docs/formatdomain.html.in >>+++ b/docs/formatdomain.html.in >>@@ -1657,6 +1657,16 @@ >> 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's a not more than 8 bytes alphanumeric string. > >Last sentence doesn't make sense, I suggest changing it to either: "It >can't be longer than 8 alphanumeric characters." or simply "Maximum 8 >alphanumeric characters." Okay, honestly, I wasn't comfortable with the sentence, but can't get a better one at that time, :-) I will change your advise a bit: It must not be longer than 8 alphanumeric characters.

> >>+<span class='since'>Since 1.0.1</span> >>+</dd> >>+<dt><code>product</code></dt> >>+<dd>If present, this element specifies the product of a virtual hard >>+ disk or CD-ROM device. It's a not more than 16 bytes alphanumeric string. > >dtto > >>+<span class='since'>Since 1.0.1</span> >>+</dd> >> <dt><code>host</code></dt> >> <dd>The<code>host</code> element has two attributes "name" and "port", >> which specify the hostname and the port number. The meaning of this >>diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng >>index 2beb035..ed7d1d0 100644 >>--- a/docs/schemas/domaincommon.rng >>+++ b/docs/schemas/domaincommon.rng >>@@ -905,6 +905,16 @@ >> <ref name="wwn"/> >> </element> >> </optional> >>+<optional> >>+<element name="vendor"> >>+<text/> >>+</element> >>+</optional> >>+<optional> >>+<element name="product"> >>+<text/> >>+</element> >>+</optional> > >This is little bit different than the other specifications around in the >code and could be made better, but looking at the schema I've found more >inconsistencies, so it's ok for now. I'll send a cleanup patch later >for these and the others as well. > >> </interleave> >> </define> >> <define name="snapshot"> >>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >>index 0575fcd..db6608e 100644 >>--- a/src/conf/domain_conf.c >>+++ b/src/conf/domain_conf.c >>@@ -979,6 +979,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); >>@@ -3498,6 +3500,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 >>@@ -3550,6 +3554,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, >> char *logical_block_size = NULL; >> char *physical_block_size = NULL; >> char *wwn = NULL; >>+ char *vendor = NULL; >>+ char *product = NULL; >> >> if (VIR_ALLOC(def)< 0) { >> virReportOOMError(); >>@@ -3888,6 +3894,24 @@ 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 bytes string")); > >This should be "disk vendor name is more than 8 characters long" or ".. >is longer than 8 characters", also there is no check these are >alphanumeric although it is mentioned in the documentation. I believe >we can use something similar to virValidateWWN(). Okay, since I already tried to validate the strings, further checking makes sense. Sorry for the late response, will send v2 soon. Regards, Osier -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list