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 <cventeic@xxxxxxxxxx>
> ---
> 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,
> - const char *model_name,
> bool migratable,
> - qemuMonitorCPUModelInfoPtr *model_info)
> + qemuMonitorCPUModelInfoPtr input,
> + qemuMonitorCPUModelInfoPtr *expansion
> + )
> {
> VIR_DEBUG("type=%d model_name=%s migratable=%d",
> - type, model_name, migratable);
> + type, input->name, migratable);
>
> QEMU_CHECK_MONITOR(mon);
>
> - return qemuMonitorJSONGetCPUModelExpansion(mon, type, model_name,
> - migratable, model_info);
> + return qemuMonitorJSONGetCPUModelExpansion(mon, type, migratable, input, expansion);
> }
>
>
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 0a09590ed1..d3efd37099 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1036,9 +1036,10 @@ typedef enum {
>
> int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon,
> qemuMonitorCPUModelExpansionType type,
> - const char *model_name,
> bool migratable,
> - qemuMonitorCPUModelInfoPtr *model_info);
> + qemuMonitorCPUModelInfoPtr input,
> + qemuMonitorCPUModelInfoPtr *expansion)
> + ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5);
>
> void qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info);
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index f20e9e9379..4e6e220a8c 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5600,12 +5600,17 @@ qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr cpu_model)
> return ret;
> }
>
> +
> +/* return:
> + * -1 - Execution Failure
> + * 0 - Success
> + */
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
> qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
> qemuMonitorCPUModelExpansionType type,
> - const char *model_name,
> bool migratable,
> - qemuMonitorCPUModelInfoPtr *model_info)
> + qemuMonitorCPUModelInfoPtr input,
> + qemuMonitorCPUModelInfoPtr *expansion)
> {
> int ret = -1;
> virJSONValuePtr json_model_in = NULL;
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
