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