[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 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.

>  
>  #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?

> +            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.

> +                if (!vendor_id
> +                    || strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) {

And || operator needs to be on the preceding line.

> +                    virCPUReportError(VIR_ERR_XML_ERROR,
> +                                      _("vendor_id must be exactly %d characters long"),

Long line.

> +                                      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.

>  
>              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


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