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

Re: [libvirt] [PATCH] Added the attribute vendor_id to the cpu model



On 03.07.2012 12:06, Michal Privoznik wrote:
On 28.06.2012 12:21, Hendrik Schwartke wrote:
Introducing the attribute vendor_id to force the CPUID instruction
in a kvm guest to return the specified vendor.
---
  docs/schemas/domaincommon.rng |    7 +++++
  src/conf/cpu_conf.c           |   64 +++++++++++++++++++++++++++++++++--------
  src/conf/cpu_conf.h           |    3 ++
  src/qemu/qemu_command.c       |    6 +++-
  tests/testutilsqemu.c         |    1 +
  5 files changed, 68 insertions(+), 13 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 46e539d..5c55269 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2820,6 +2820,13 @@
            </choice>
          </attribute>
        </optional>
+<optional>
+<attribute name="vendor_id">
+<data type="string">
+<param name='pattern'>[^,]{12}</param>
+</data>
+</attribute>
+</optional>
        <choice>
          <text/>
          <empty/>
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index b520f7c..b3098d8 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -22,6 +22,7 @@
   */

  #include<config.h>
+#include<c-ctype.h>
This is useless.
Oops! I overlooked that.

  #include "virterror_internal.h"
  #include "memory.h"
@@ -68,6 +69,7 @@ virCPUDefFreeModel(virCPUDefPtr def)

      VIR_FREE(def->model);
      VIR_FREE(def->vendor);
+    VIR_FREE(def->vendor_id);

      for (i = 0; i<  def->nfeatures; i++)
          VIR_FREE(def->features[i].name);
@@ -104,6 +106,7 @@ virCPUDefCopyModel(virCPUDefPtr dst,

      if ((src->model&&  !(dst->model = strdup(src->model)))
          || (src->vendor&&  !(dst->vendor = strdup(src->vendor)))
+        || (src->vendor_id&&  !(dst->vendor_id = strdup(src->vendor_id)))
          || VIR_ALLOC_N(dst->features, src->nfeatures)<  0)
          goto no_memory;
      dst->nfeatures_max = dst->nfeatures = src->nfeatures;
@@ -288,18 +291,46 @@ virCPUDefParseXML(const xmlNodePtr node,
      }

      if (def->type == VIR_CPU_TYPE_GUEST&&
-        def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH&&
-        virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) {
-        const char *fallback;
-
-        fallback = virXPathString("string(./model[1]/@fallback)", ctxt);
-        if (fallback) {
-            def->fallback = virCPUFallbackTypeFromString(fallback);
-            VIR_FREE(fallback);
-            if (def->fallback<  0) {
-                virCPUReportError(VIR_ERR_XML_ERROR, "%s",
-                                  _("Invalid fallback attribute"));
-                goto error;
+        def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) {
+
+        if (virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) {
+            const char *fallback;
+
+            fallback =
+                virXPathString("string(./model[1]/@fallback)", ctxt);
Why this change?
Well, Eric suggested to use 'make syntax-check'. And it recommended to use two lines.
+            if (fallback) {
+                def->fallback = virCPUFallbackTypeFromString(fallback);
+                VIR_FREE(fallback);
+                if (def->fallback<  0) {
+                    virCPUReportError(VIR_ERR_XML_ERROR, "%s",
+                                      _("Invalid fallback attribute"));
+                    goto error;
+                }
+            }
+
+            if (virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) {
+                char *vendor_id;
+
+                vendor_id =
+                    virXPathString("string(./model[1]/@vendor_id)", ctxt);
If we are dealing with long lines we tend to split them rather at ',' than this.
I think make syntax-check didn't like the long line here.
+                if (!vendor_id
+                    || strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) {
And || operator needs to be on the preceding line.
Again the syntax check recommended something else. There are also a lot of other places in the code where operators are at the beginning of a line, see above. (IMHO it's easier to read.)
+                    virCPUReportError(VIR_ERR_XML_ERROR,
+                                      _("vendor_id must be exactly %d characters long"),
Long line.
Yep, your're right
+                                      VIR_CPU_VENDOR_ID_LENGTH);
+                    VIR_FREE(vendor_id);
+                    goto error;
+                }
+                /* ensure that the string can be passed to qemu*/
+                for (i = 0; i<  strlen(vendor_id); i++) {
+                    if (vendor_id[i]==',') {
+                        virCPUReportError(VIR_ERR_XML_ERROR, "%s",
+                                          _("vendor id is invalid"));
+                        VIR_FREE(vendor_id);
+                        goto error;
+                    }
+                }
+                def->vendor_id = vendor_id;
              }
          }
      }
@@ -588,6 +619,8 @@ virCPUDefFormatBuf(virBufferPtr buf,
                  return -1;
              }
              virBufferAsprintf(buf, " fallback='%s'", fallback);
+            if (def->vendor_id)
+                virBufferAsprintf(buf, " vendor_id='%s'", def->vendor_id);
          }
          if (formatModel&&  def->model) {
              virBufferAsprintf(buf, ">%s</model>\n", def->model);
@@ -738,6 +771,13 @@ virCPUDefIsEqual(virCPUDefPtr src,
          goto cleanup;
      }

+    if (STRNEQ_NULLABLE(src->vendor_id, dst->vendor_id)) {
+        virCPUReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                          _("Target CPU model %s does not match source %s"),
+                          NULLSTR(dst->vendor_id), NULLSTR(src->vendor_id));
+        goto cleanup;
+    }
+
      if (src->sockets != dst->sockets) {
          virCPUReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                            _("Target CPU sockets %d does not match source %d"),
diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h
index f8b7bf9..2df0a50 100644
--- a/src/conf/cpu_conf.h
+++ b/src/conf/cpu_conf.h
@@ -28,6 +28,8 @@
  # include "buf.h"
  # include "xml.h"

+# define VIR_CPU_VENDOR_ID_LENGTH 12
+
  enum virCPUType {
      VIR_CPU_TYPE_HOST,
      VIR_CPU_TYPE_GUEST,
@@ -103,6 +105,7 @@ struct _virCPUDef {
      int match;          /* enum virCPUMatch */
      char *arch;
      char *model;
+    char *vendor_id;    /* vendor id returned by CPUID in the guest */
      int fallback;       /* enum virCPUFallback */
      char *vendor;
      unsigned int sockets;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bd4f96a..d8d0220 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3910,7 +3910,9 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver,
              }
              virBufferAddLit(&buf, "host");
          } else {
-            if (VIR_ALLOC(guest)<  0 || !(guest->arch = strdup(host->arch)))
+            if (VIR_ALLOC(guest)<  0
+                || !(guest->arch = strdup(host->arch))
+                || (cpu->vendor_id&&  !(guest->vendor_id = strdup(cpu->vendor_id))))
                  goto no_memory;
Again operators on the begining of line.
Ok, get that.

              if (cpu->match == VIR_CPU_MATCH_MINIMUM)
@@ -3924,6 +3926,8 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver,
                  goto cleanup;

              virBufferAdd(&buf, guest->model, -1);
+            if (guest->vendor_id)
+                virBufferAsprintf(&buf, ",vendor=%s", guest->vendor_id);
              for (i = 0; i<  guest->nfeatures; i++) {
                  char sign;
                  if (guest->features[i].policy == VIR_CPU_FEATURE_DISABLE)
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index 8d5a3bf..8b7cb33 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -116,6 +116,7 @@ virCapsPtr testQemuCapsInit(void) {
          0,                      /* match */
          (char *) "x86_64",      /* arch */
          (char *) "core2duo",    /* model */
+        NULL,                   /* vendor_id */
          0,                      /* fallback */
          (char *) "Intel",       /* vendor */
          1,                      /* sockets */


So squashing this in:

diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index b3098d8..7fe3c1e 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -22,7 +22,6 @@
   */

  #include<config.h>
-#include<c-ctype.h>

  #include "virterror_internal.h"
  #include "memory.h"
@@ -296,8 +295,7 @@ virCPUDefParseXML(const xmlNodePtr node,
          if (virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) {
              const char *fallback;

-            fallback =
-                virXPathString("string(./model[1]/@fallback)", ctxt);
+            fallback = virXPathString("string(./model[1]/@fallback)", ctxt);
              if (fallback) {
                  def->fallback = virCPUFallbackTypeFromString(fallback);
                  VIR_FREE(fallback);
@@ -311,12 +309,13 @@ virCPUDefParseXML(const xmlNodePtr node,
              if (virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) {
                  char *vendor_id;

-                vendor_id =
-                    virXPathString("string(./model[1]/@vendor_id)", ctxt);
-                if (!vendor_id
-                    || strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) {
+                vendor_id = virXPathString("string(./model[1]/@vendor_id)",
+                                           ctxt);
+                if (!vendor_id ||
+                    strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) {
                      virCPUReportError(VIR_ERR_XML_ERROR,
-                                      _("vendor_id must be exactly %d characters long"),
+                                      _("vendor_id must be exactly"
+                                        " %d characters long"),
                                        VIR_CPU_VENDOR_ID_LENGTH);
                      VIR_FREE(vendor_id);
                      goto error;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0b5d894..528b189 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3913,9 +3913,9 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver,
              }
              virBufferAddLit(&buf, "host");
          } else {
-            if (VIR_ALLOC(guest)<  0
-                || !(guest->arch = strdup(host->arch))
-                || (cpu->vendor_id&&  !(guest->vendor_id = strdup(cpu->vendor_id))))
+            if (VIR_ALLOC(guest)<  0 ||
+                !(guest->arch = strdup(host->arch)) ||
+                (cpu->vendor_id&&  !(guest->vendor_id = strdup(cpu->vendor_id))))
                  goto no_memory;

              if (cpu->match == VIR_CPU_MATCH_MINIMUM)


And pushed now.

Michal
Thanks a lot Michal!

Hendrik


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