[libvirt] [PATCH] Added the attribute vendor_id to the cpu model
Hendrik Schwartke
hendrik at os-t.de
Tue Jul 3 10:37:58 UTC 2012
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
More information about the libvir-list
mailing list