On 07/30/2013 03:09 PM, Don Dugger wrote:
[V2 - per review comments change the flag name to be more descriptive and change errors from warnings to propogated errors.]
This aside belongs...
Currently the virConnectBaselineCPU API does not expose the CPU features that are part of the CPU's model. This patch adds a new flag, VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURE, that causes the API to explictly list all features that are part of that model.
Signed-off-by: Don Dugger <donald.d.dugger@intel.com> ---
...here, after the '---', so that 'git am' won't turn it into a permanent part of libvirt.git history. Your patch didn't apply as is (are you properly rebasing to the latest libvirt.git?), but the conflict resolution in qemu_driver.c seemed pretty trivial. You also failed 'make syntax-check': flags_usage src/cpu/cpu_arm.c:48: unsigned int flags ATTRIBUTE_UNUSED) src/cpu/cpu_generic.c:117: unsigned int flags ATTRIBUTE_UNUSED) src/cpu/cpu_powerpc.c:308: unsigned int flags ATTRIBUTE_UNUSED) src/cpu/cpu_powerpc.c:382: unsigned int flags ATTRIBUTE_UNUSED) src/cpu/cpu_s390.c:52: unsigned int flags ATTRIBUTE_UNUSED) maint.mk: flags should be checked with virCheckFlags When adding a flags parameter, we intentionally update callers to explicitly check for a 0 argument if they are unprepared to handle non-zero flags; this leaves the door open for extensions that use new flag bits, rather than being stuck with undefined behavior when passing a new bit to an old binary. So, all of these sites need to use either virCheckFlags(0, NULL) to state they don't handle the flag, or virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURE, NULL) to state that the flag is valid (and does not impact the output for that architecture).
include/libvirt/libvirt.h.in | 9 ++++++
Needs a corresponding doc patch in src/libvirt.c that mentions use of the new flag.
tools/virsh-domain.c | 11 ++++++-
Likewise, this needs a corresponding patch to tools/virsh.pod to update the man page to mention the new virsh option.
+++ b/include/libvirt/libvirt.h.in @@ -3890,6 +3890,15 @@ int virConnectCompareCPU(virConnectPtr conn,
/** + * virConnectBaselineCPUFlags + * + * Flags when getting XML description of a computed CPU + */ +typedef enum { + VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURE = (1 << 0), /* show model features*/
Space before */ I know this is the name Dan suggested, but I can't help but wonder if _EXPAND_FEATURES sounds nicer than EXPAND_FEATURE, since pretty much every cpu name has more than one feature listed when expanded.
+++ b/src/cpu/cpu.c @@ -167,7 +167,7 @@ cpuDecode(virCPUDefPtr cpu, return -1; }
- return driver->decode(cpu, data, models, nmodels, preferred); + return driver->decode(cpu, data, models, nmodels, preferred, 0); }
@@ -277,7 +277,8 @@ char * cpuBaselineXML(const char **xmlCPUs, unsigned int ncpus, const char **models, - unsigned int nmodels) + unsigned int nmodels, + unsigned int flags) {
Needs to make sure we reject invalid flags: virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURE, -1)
+++ b/src/cpu/cpu.h @@ -49,7 +49,8 @@ typedef int const union cpuData *data, const char **models, unsigned int nmodels, - const char *preferred); + const char *preferred, + unsigned int flags);
typedef int (*cpuArchEncode) (const virCPUDefPtr cpu, @@ -76,7 +77,8 @@ typedef virCPUDefPtr (*cpuArchBaseline) (virCPUDefPtr *cpus, unsigned int ncpus, const char **models, - unsigned int nmodels); + unsigned int nmodels, + unsigned int /* flags */);
No need to comment out the parameter name (multiple instances */.
+++ b/src/cpu/cpu_x86.c @@ -1296,13 +1296,46 @@ x86GuestData(virCPUDefPtr host, return x86Compute(host, guest, data, message); }
+static int +x86AddFeatures(virCPUDefPtr cpu, + struct x86_map *map) +{ + const struct x86_model *candidate; + const struct x86_feature *feature = map->features; + + candidate = map->models; + while (candidate != NULL) { + if (STREQ(cpu->model, candidate->name)) + break; + candidate = candidate->next; + } + if (!candidate) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s not a known CPU model\n", cpu->model); + return -1; + } + while (feature != NULL) { + if (x86DataIsSubset(candidate->data, feature->data)) { + if (virCPUDefAddFeature(cpu, feature->name, VIR_CPU_FEATURE_REQUIRE) < 0) {
This function already outputs a decent error message on failure...
+ virReportError(VIR_ERR_INTERNAL_ERROR, + "CPU model %s, no room for feature %s", + cpu->model, feature->name);
...but this message overwrites that error. Drop this line.
@@ -1383,6 +1416,9 @@ x86Decode(virCPUDefPtr cpu, goto out; }
+ if (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURE) + if (x86AddFeatures(cpuModel, map) < 0) + goto out;
You could write this with fewer nested if: if ((flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURE) && x86AddFeatures(cpuModel, map) < 0) goto out;
+++ b/tests/cputest.c
#define DO_TEST(arch, api, name, host, cpu, \ - models, nmodels, preferred, result) \ + models, nmodels, preferred, flags, result) \ do { \ static struct data data = { \ arch, api, host, cpu, models, \ models == NULL ? NULL : #models, \ - nmodels, preferred, result \ + nmodels, preferred, flags, result \
Might as well fix the alignment of the \ while touching this.
+++ b/tools/virsh-domain.c @@ -5986,6 +5986,10 @@ static const vshCmdOptDef opts_cpu_baseline[] = { .flags = VSH_OFLAG_REQ, .help = N_("file containing XML CPU descriptions") }, + {.name = "model_features",
virsh flags should favor - over _. But rather than name it --model-features, why not just go with the shorter --features? Furthermore, the fact that you named it "features" rather than "feature" adds weight to my argument that the flag name in libvirt.h.in needs to use _FEATURES.
@@ -6049,7 +6057,8 @@ cmdCPUBaseline(vshControl *ctl, const vshCmd *cmd) list[i] = vshStrdup(ctl, (const char *)xmlBufferContent(xml_buf)); }
- result = virConnectBaselineCPU(ctl->conn, list, count, 0); + result = virConnectBaselineCPU(ctl->conn, list, count, flags); +vshPrint(ctl, "result - %p\n", result);
Leftover debugging?
if (result) { vshPrint(ctl, "%s", result);
Getting closer; looking forward to v3. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org