4:59 p.m.
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(a)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