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@xxxxxxxxx>
> ---
...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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list