[libvirt] [PATCH] Expose all CPU features in host definition

[Martin Kletzander]

On 05/25/2013 12:44 AM, Don Dugger wrote:
> I've opened BZ 697141 on this as I would consider it more
> a bug than a feature request.  Anyway, to re-iterate my
> rationale from the BZ:
> 
> 
> The virConnectGetCapabilities API describes the host capabilities
> by returning an XML description that includes the CPU model name
> and a set of CPU features.  The problem is that any features that
> are part of the CPU model are not explicitly listed, they are
> assumed to be part of the definition of that CPU model.  This
> makes it extremely difficult for the caller of this API to check
> for the presence of a specific CPU feature, the caller would have
> to know what features are part of which CPU models, a very
> daunting task.
> 
> This patch solves this problem by having the API return a model
> name, as it currently does, but it will also explicitly list all
> of the CPU features that are present.  This would make it much
> easier for a caller of this API to check for specific features.
> 
> Signed-off-by: Don Dugger <donald.d.dugger at intel.com>
> 

I'm generally not against exposing CPU model features in capabilities,
but if we do this, such features should be distinguishable from those
not in the model.  Of course we don't want users to go to
/usr/share/libvirt/cpu_map.xml all the time, but maybe there could be
separate API for that.  If not, then it should be encapsulated somewhere
else than side by side the other features.

> ---
>  src/cpu/cpu_x86.c |   30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> index 5d479c2..b2e16df 100644
> --- a/src/cpu/cpu_x86.c
> +++ b/src/cpu/cpu_x86.c
> @@ -1296,6 +1296,35 @@ x86GuestData(virCPUDefPtr host,
>      return x86Compute(host, guest, data, message);
>  }
>  
> +static void
> +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))

Don't indent with TABs, there's even a 'make syntax-check' rule for that.

> +	    break;
> +	candidate = candidate->next;
> +    }
> +    if (!candidate) {
> +	VIR_WARN("Odd, %s not a known CPU model\n", cpu->model);
> +	return;

Warning seems inappropriate here as this is actually an error.

> +    }
> +    while (feature != NULL) {
> +	if (x86DataIsSubset(candidate->data, feature->data)) {
> +	    if (virCPUDefAddFeature(cpu, feature->name, VIR_CPU_FEATURE_DISABLE) < 0) {
> +		VIR_WARN("CPU model %s, no room for feature %s", cpu->model, feature->name);
> +		return;

This code path shadows an error and means the feature will not be
mentioned in the capabilities, but the API will end successfully.

Martin