[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]