[libvirt] [PATCH v3] Add flag to BaselineCPU API to return detailed CPU features

[Eric Blake]

On 08/02/2013 01:08 PM, Don Dugger wrote:
> 
> 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_FEATURES, that causes the API to explictly

s/explictly/explicitly

> list all features that are part of that model.
> 
> Signed-off-by: Don Dugger <donald.d.dugger at intel.com>
> 
> ---
> [V2 - per review comments change the flag name to be more descriptive
> and change errors from warnings to propogated errors.]
> 
> [V3 - Incorporate review suggestions to better handle flags and errors.
> Also add documentation about the API change.]

> +++ b/src/cpu/cpu_x86.c
> @@ -1319,13 +1319,41 @@ 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"), cpu->model);
> +        return -1;
> +    }
> +    while (feature != NULL) {
> +        if (x86DataIsSubset(candidate->data, feature->data) &&
> +            (virCPUDefAddFeature(cpu, feature->name, VIR_CPU_FEATURE_REQUIRE) < 0))

The outer () aren't needed on this line.

> +                return -1;

Indentation is off.


> @@ -1406,6 +1436,9 @@ x86Decode(virCPUDefPtr cpu,
>          goto out;
>      }
>  
> +    if (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES &&
> +        (x86AddFeatures(cpuModel, map) < 0))

Extra ()

> +            goto out;

Indentation is off.

> +++ b/tools/virsh-domain.c
> @@ -6148,6 +6148,10 @@ static const vshCmdOptDef opts_cpu_baseline[] = {
>       .flags = VSH_OFLAG_REQ,
>       .help = N_("file containing XML CPU descriptions")
>      },
> +    {.name = "features",

> @@ -6168,6 +6173,9 @@ cmdCPUBaseline(vshControl *ctl, const vshCmd *cmd)
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>      size_t i;
>  
> +    if (vshCommandOptBool(cmd, "model_features"))
> +        flags |= VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES;

Mismatch in names.  How did you even test this?  My testing: I used
just-compiled virsh with existing libvirtd, and was surprised to get no
failure and no difference in output; after tweaking the virsh bug, I
finally got expected failure:
$ tools/virsh cpu-baseline ../foo.xml --features
error: unsupported flags (0x1) in function qemuConnectBaselineCPU

then after restarting to just-built libvirtd, I checked that adding
--features changed the output.

(hmm, maybe virsh can add some sanity checking code to make sure we
aren't querying for a mismatched name; but that would be a followup
patch...)

Those are all minor, so I squashed this in and pushed.  Thanks again for
being patient, and for the pings along the way.

diff --git i/src/cpu/cpu_x86.c w/src/cpu/cpu_x86.c
index f16a3cb..41ce21f 100644
--- i/src/cpu/cpu_x86.c
+++ w/src/cpu/cpu_x86.c
@@ -1339,8 +1339,9 @@ x86AddFeatures(virCPUDefPtr cpu,
     }
     while (feature != NULL) {
         if (x86DataIsSubset(candidate->data, feature->data) &&
-            (virCPUDefAddFeature(cpu, feature->name,
VIR_CPU_FEATURE_REQUIRE) < 0))
-                return -1;
+            virCPUDefAddFeature(cpu, feature->name,
+                                VIR_CPU_FEATURE_REQUIRE) < 0)
+            return -1;
         feature = feature->next;
     }
     return 0;
@@ -1437,8 +1438,8 @@ x86Decode(virCPUDefPtr cpu,
     }

     if (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES &&
-        (x86AddFeatures(cpuModel, map) < 0))
-            goto out;
+        x86AddFeatures(cpuModel, map) < 0)
+        goto out;
     cpu->model = cpuModel->model;
     cpu->vendor = cpuModel->vendor;
     cpu->nfeatures = cpuModel->nfeatures;
diff --git i/tools/virsh-domain.c w/tools/virsh-domain.c
index 7d31622..13e3045 100644
--- i/tools/virsh-domain.c
+++ w/tools/virsh-domain.c
@@ -6182,7 +6182,7 @@ cmdCPUBaseline(vshControl *ctl, const vshCmd *cmd)
     virBuffer buf = VIR_BUFFER_INITIALIZER;
     size_t i;

-    if (vshCommandOptBool(cmd, "model_features"))
+    if (vshCommandOptBool(cmd, "features"))
         flags |= VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES;

     if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0)


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130816/89063427/attachment-0001.sig>