[libvirt] [PATCH v2 3/7] cpu_x86: Introduce virCPUx86FeatureIsMSR

[Ján Tomko]

On Thu, Jun 20, 2019 at 12:53:38AM +0200, Jiri Denemark wrote:
>This function may be used as a virCPUDefFeatureFilter callback for
>virCPUDefCheckFeatures, virCPUDefFilerFeatures, and similar functions to
>filter or pick out features reported via MSR.
>
>Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
>---
> src/cpu/cpu_x86.c        | 40 ++++++++++++++++++++++++++++++++++++++++
> src/cpu/cpu_x86.h        |  3 +++
> src/libvirt_private.syms |  1 +
> 3 files changed, 44 insertions(+)
>
>diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
>index 6eef5cef00..a7ec0f7095 100644
>--- a/src/cpu/cpu_x86.c
>+++ b/src/cpu/cpu_x86.c
>@@ -3362,6 +3362,46 @@ virCPUx86DataAddFeature(virCPUDataPtr cpuData,
> }
>
>
>+/**
>+ * virCPUx86FeatureIsMSR:
>+ * @name CPU feature name
>+ * @opaque NULL or a pointer to bool
>+ *
>+ * This is a callback for functions filtering features in virCPUDef.
>+ *
>+ * Checks whether a given CPU feature is reported via MSR. When @opaque is NULL
>+ * or a pointer to true, the function will pick out (return true for) MSR
>+ * features. If @opaque is a pointer to false, the logic will be inverted and
>+ * the function will filter out (return false for) MSR features.
>+ */

Uhm. What? Do I understand it correctly?

@opaque | *opaque | IsMSR | return
--------+---------+-------+--------
NULL    | SEGV    | true  | true
0xBEEF  | true    | true  | true
0xBEEF  | false   | true  | false
--------+---------+-------+--------

First, it feels odd that 'false' is the value resulting in
changed behavior. I'd rather see it act differently when the bool
pointer is present and points to true.

Second, by using a pointer to bool AND not requiring opaque to be
passed, we essentially have a tri-state bool.

I'd suggest (ab)using the void pointer to store the value directly,
we do have the
  bool var = opaque != NULL;
pattern elsewhere in the code.

>+bool
>+virCPUx86FeatureIsMSR(const char *name,
>+                      void *opaque)

Can you name this virCPUx86FeatureFilterMSR, leaving virCPUx86FeatureIsMSR
for a static helper function that will perform the actual lookup and
this function would (possibly) invert the result? All the ternary
operators make the function harder to follow.

>+{
>+    virCPUx86FeaturePtr feature;
>+    virCPUx86DataIterator iter;
>+    virCPUx86DataItemPtr item;
>+    virCPUx86MapPtr map;
>+    bool inverted = false;
>+
>+    if (opaque)
>+        inverted = !*(bool *)opaque;
>+
>+    if ((!(map = virCPUx86GetMap()) ||

This condition would be more readable if it were split in two.

>+         !(feature = x86FeatureFind(map, name))) &&
>+        !(feature = x86FeatureFindInternal(name)))
>+        return inverted ? true : false;
>+
>+    virCPUx86DataIteratorInit(&iter, &feature->data);
>+    while ((item = virCPUx86DataNext(&iter))) {
>+        if (item->type == VIR_CPU_X86_DATA_MSR)
>+            return inverted ? false : true;
>+    }
>+
>+    return inverted ? true : false;
>+}
>+
>+
> struct cpuArchDriver cpuDriverX86 = {
>     .name = "x86",
>     .arch = archs,

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190620/2c5c68e8/attachment-0001.sig>