Re: [libvirt] [PATCH v3 3/6] qemu_monitor: qemuMonitorGetCPUModelExpansion inputs and outputs CPUModelInfo
On Thu, Sep 27, 2018 at 16:26:42 -0500, Chris Venteicher wrote:
> A Full CPUModelInfo structure with props is sent to QEMU for expansion.
>
> virQEMUCapsProbeQMPHostCPU migratability logic partitioned into new function
> for clarity.
Unrelated changes should go into separate patches. Please, split this
patch in two. Mixing several separable changes in a single patch makes
it harder to review.
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_capabilities.c | 125 ---
> src/qemu/qemu_monitor.c | 47 +++--
> src/qemu/qemu_monitor.h | 5 +-
> src/qemu/qemu_monitor_json.c | 20 --
> src/qemu/qemu_monitor_json.h | 6 +-
> tests/cputest.c | 11 ++-
> 6 files changed, 156 insertions(+), 58 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index e60a4b369e..d38530ca80 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2352,15 +2352,82 @@ virQEMUCapsProbeQMPCPUDefinitions(virQEMUCapsPtr
> qemuCaps,
> return 0;
> }
>
> +/* virQEMUCapsMigratablePropsDiff
> + * @migratable: migratable props=true, non-migratable & unsupported
> props=false
> + * @nonMigratable: migratable & non-migratable props = true, unsupported
> props = false
> + * @augmented: prop->migratable = VIR_TRISTATE_BOOL_{YES/NO} base on diff
> + *
> + * Use differences in Expanded CPUModelInfo inputs
> + * to augment with prop->migratable in CPUModelInfo output
Could you use normal language to describe the parameters and what the
function is supposed to do. Having to read the documentation over and
over and thinking hard about it to grasp what the function is supposed
to do is even worse than studying the code itself.
The function and parameters names look like they give better idea about
what's going on here than the comments which are supposed to explain
them.
> + */
> +static int
> +virQEMUCapsMigratablePropsDiff(qemuMonitorCPUModelInfoPtr migratable,
> + qemuMonitorCPUModelInfoPtr nonMigratable,
> + qemuMonitorCPUModelInfoPtr *augmented)
...
> cleanup:
> -virHashFree(hash);
> +qemuMonitorCPUModelInfoFree(input);
> +qemuMonitorCPUModelInfoFree(migratable);
> qemuMonitorCPUModelInfoFree(nonMigratable);
> -qemuMonitorCPUModelInfoFree(modelInfo);
> +qemuMonitorCPUModelInfoFree(augmented);
>
> return ret;
> }
This is where the first patch would end. Although you'd need to change
it a bit to account for the missing changes which will go in later as a
second patch.
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 801c072eff..c64b3ad38a 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3653,20 +3653,57 @@ qemuMonitorCPUDefInfoFree(qemuMonitorCPUDefInfoPtr
> cpu)
> }
>
>
> +/**
> + * qemuMonitorGetCPUModelExpansion:
> + * @mon:
> + * @type: qemuMonitorCPUModelExpansionType
> + * @migratable: Prompt QEMU to include non-migratable features for X86
> models if false.
> + * @input: Non-expanded input model
> + * @expansion: Expanded output model (or NULL if QEMU rejects model /
> request)
> + *
> + * CPU property lists are computed from CPUModelInfo structures by 1)
> converting
> + * model->name into a property list using a static lookup table and 2)
> adding or
> + * removing properties in the resulting list from model->props.
> + *
> + * This function uses QEMU to identify a base model name that most closely
> + * matches the migratable CPU properties in the input CPU Model.
> + *
> + * This function also uses QEMU to enumerate model->props in various ways
> based
> + * on the qemuMonitorCPUModelExpansionType.
> + *
> + * full_input_props = LookupProps(input->name) then +/- input->props
> + *
> + * migratable_input_props = full_input_props - non_migratable_input_props
> + *
> + * base_model = FindClosestBaseModel(migratable_input_props)
> + *
> + * expansion->name = base_model->name
> + *
> + * @type == QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC
> + * expansion->props = props to +/- to base_model to approximate
> migratable_input_props
> + *
> + * @type == QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL
> + * expansion->props = full_input_props
> + *
> + * @type == QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL
> + * expansion->props = migratable_input_props
Same comment as for the other doc text above. I know what the function
is supposed to do, but still I find it hard to understand what you
wanted to say with all the text above.
> + *
> + * Returns 0 in case of success, -1 in case of failure
> + */
This applies to almost all functions in libvirt. There's no sense in
documenting it unless you add more documentation for that function and
mention return values for completeness.
> int
> qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon,
> qemuMonitorCPUModelExpansionType type,
> -
[libvirt] [PATCH v3 3/6] qemu_monitor: qemuMonitorGetCPUModelExpansion inputs and outputs CPUModelInfo
A Full CPUModelInfo structure with props is sent to QEMU for expansion.
virQEMUCapsProbeQMPHostCPU migratability logic partitioned into new function
for clarity.
Signed-off-by: Chris Venteicher
---
src/qemu/qemu_capabilities.c | 125 ---
src/qemu/qemu_monitor.c | 47 +++--
src/qemu/qemu_monitor.h | 5 +-
src/qemu/qemu_monitor_json.c | 20 --
src/qemu/qemu_monitor_json.h | 6 +-
tests/cputest.c | 11 ++-
6 files changed, 156 insertions(+), 58 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e60a4b369e..d38530ca80 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2352,15 +2352,82 @@ virQEMUCapsProbeQMPCPUDefinitions(virQEMUCapsPtr
qemuCaps,
return 0;
}
+/* virQEMUCapsMigratablePropsDiff
+ * @migratable: migratable props=true, non-migratable & unsupported props=false
+ * @nonMigratable: migratable & non-migratable props = true, unsupported props
= false
+ * @augmented: prop->migratable = VIR_TRISTATE_BOOL_{YES/NO} base on diff
+ *
+ * Use differences in Expanded CPUModelInfo inputs
+ * to augment with prop->migratable in CPUModelInfo output
+ */
+static int
+virQEMUCapsMigratablePropsDiff(qemuMonitorCPUModelInfoPtr migratable,
+ qemuMonitorCPUModelInfoPtr nonMigratable,
+ qemuMonitorCPUModelInfoPtr *augmented)
+{
+int ret = -1;
+qemuMonitorCPUModelInfoPtr tmp;
+qemuMonitorCPUPropertyPtr prop;
+qemuMonitorCPUPropertyPtr mProp;
+qemuMonitorCPUPropertyPtr nmProp;
+virHashTablePtr hash = NULL;
+size_t i;
+
+*augmented = NULL;
+
+if (!(tmp = qemuMonitorCPUModelInfoCopy(migratable)))
+goto cleanup;
+
+if (!nonMigratable)
+goto done;
+
+if (!(hash = virHashCreate(0, NULL)))
+goto cleanup;
+
+for (i = 0; i < tmp->nprops; i++) {
+prop = tmp->props + i;
+
+if (virHashAddEntry(hash, prop->name, prop) < 0)
+goto cleanup;
+}
+
+for (i = 0; i < nonMigratable->nprops; i++) {
+nmProp = nonMigratable->props + i;
+
+if (!(mProp = virHashLookup(hash, nmProp->name)) ||
+mProp->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN ||
+mProp->type != nmProp->type)
+continue; /* In non-migratable list but not in migratable list */
+
+if (mProp->value.boolean) {
+mProp->migratable = VIR_TRISTATE_BOOL_YES;
+} else if (nmProp->value.boolean) {
+mProp->value.boolean = true;
+mProp->migratable = VIR_TRISTATE_BOOL_NO;
+}
+}
+
+tmp->migratability = true;
+
+ done:
+VIR_STEAL_PTR(*augmented, tmp);
+ret = 0;
+
+ cleanup:
+qemuMonitorCPUModelInfoFree(tmp);
+virHashFree(hash);
+return ret;
+}
static int
virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
qemuMonitorPtr mon,
bool tcg)
{
-qemuMonitorCPUModelInfoPtr modelInfo = NULL;
+qemuMonitorCPUModelInfoPtr input;
+qemuMonitorCPUModelInfoPtr migratable = NULL;
qemuMonitorCPUModelInfoPtr nonMigratable = NULL;
-virHashTablePtr hash = NULL;
+qemuMonitorCPUModelInfoPtr augmented = NULL;
const char *model;
qemuMonitorCPUModelExpansionType type;
virDomainVirtType virtType;
@@ -2380,6 +2447,8 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
cpuData = virQEMUCapsGetHostCPUData(qemuCaps, virtType);
+cpuData->info = NULL;
+
/* Some x86_64 features defined in cpu_map.xml use spelling which differ
* from the one preferred by QEMU. Static expansion would give us only the
* preferred spelling, thus we need to do a full expansion on the result of
@@ -2390,54 +2459,30 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
else
type = QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC;
-if (qemuMonitorGetCPUModelExpansion(mon, type, model, true, ) <
0)
+if (!(input = qemuMonitorCPUModelInfoNew(model)) ||
+qemuMonitorGetCPUModelExpansion(mon, type, true, input, ) <
0)
goto cleanup;
-/* Try to check migratability of each feature. */
-if (modelInfo &&
-qemuMonitorGetCPUModelExpansion(mon, type, model, false,
-) < 0)
+if (!migratable) {
+ret = 0; /* Qemu can't expand the model name, exit without error
*/
goto cleanup;
+}
-if (nonMigratable) {
-qemuMonitorCPUPropertyPtr prop;
-qemuMonitorCPUPropertyPtr nmProp;
-size_t i;
-
-if (!(hash = virHashCreate(0, NULL)))
-goto cleanup;
-
-for (i = 0; i < modelInfo->nprops; i++) {
-prop = modelInfo->props + i;
-if (virHashAddEntry(hash, prop->name, prop) < 0)
-goto cleanup;
-}
-
-for (i = 0; i < nonMigratable->nprops; i++) {
-nmProp =
