[libvirt] [PATCH v2 3/7] cpu_x86: Introduce virCPUx86FeatureIsMSR
Ján Tomko
jtomko at redhat.com
Thu Jun 20 08:16:36 UTC 2019
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>
More information about the libvir-list
mailing list