On Wed, Nov 07, 2012 at 11:24:43PM +0800, Osier Yang wrote:
> 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.
Not to be pedantic, but what happens if you hand it doublebyte
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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list