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

[Dave Allan]

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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list