[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2] Add flag to BaselineCPU API to return detailed CPU features



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]