[libvirt] [PATCH v2 1/8] qemu_monitor: helper functions for CPU models

[Collin Walling]

On 4/29/19 10:08 AM, Daniel Henrique Barboza wrote:
> 
> 
> On 4/26/19 5:22 PM, walling at linux.ibm.com wrote:
>> From: Collin Walling<walling at linux.ibm.com>
>>
>> Refactor some code in qemuMonitorJSONGetCPUModelExpansion to be
>> later used for the comparison and baseline functions.
>>
>> This does not alter any functionality.
>>
>> Signed-off-by: Collin Walling<walling at linux.ibm.com>
>> Reviewed-by: Bjoern Walk<bwalk at linux.ibm.com>
>> ---
>>   src/qemu/qemu_monitor_json.c | 170 
>> ++++++++++++++++++++++++++++++-------------
>>   1 file changed, 118 insertions(+), 52 deletions(-)
>>
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index 908967f..9618d8e 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -5513,6 +5513,118 @@ qemuMonitorJSONParseCPUModelProperty(const 
>> char *key,
>>       return 0;
>>   }
>> +
>> +static virJSONValuePtr
>> +qemuMonitorJSONMakeCPUModel(const char *model_name,
>> +                            size_t nprops,
>> +                            virCPUFeatureDefPtr props,
>> +                            bool migratable)
>> +{
>> +    virJSONValuePtr value;
>> +    virJSONValuePtr feats = NULL;
>> +    size_t i;
>> +
>> +    if (!(value = virJSONValueNewObject()))
>> +        goto cleanup;
>> +
>> +    if (virJSONValueObjectAppendString(value, "name", model_name) < 0)
>> +        goto cleanup;
>> +
>> +    if (nprops || !migratable) {
>> +        if (!(feats = virJSONValueNewObject()))
>> +            goto cleanup;
>> +
>> +        for (i = 0; i < nprops; i++) {
>> +            char *name = props[i].name;
>> +            bool enabled = false;
>> +
>> +            if (props[i].policy == VIR_CPU_FEATURE_REQUIRE ||
>> +                props[i].policy == VIR_CPU_FEATURE_FORCE)
>> +                enabled = true;
>> +
>> +            if (virJSONValueObjectAppendBoolean(feats, name, enabled) 
>> < 0)
>> +                goto cleanup;
>> +        }
>> +
>> +        if (!migratable) {
>> +            if (virJSONValueObjectAppendBoolean(feats, "migratable", 
>> false) < 0)
>> +            goto cleanup;
> 
> Missing indentation of 'goto cleanup' after the if clause.
> 
> 

Good eye, thanks!

>> +        }
>> +
>> +        if (virJSONValueObjectAppend(value, "props", feats) < 0)
>> +            goto cleanup;
>> +    }
>> +
>> +    return value;
>> +
>> + cleanup:
>> +    virJSONValueFree(value);
>> +    virJSONValueFree(feats);
>> +    return NULL;
>> +}
>> +

[...]

>> -
>> -    if (!(cpu_props = virJSONValueObjectGetObject(cpu_model, 
>> "props"))) {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> -                       _("query-cpu-model-expansion reply data was 
>> missing 'props'"));
>> -        goto cleanup;
>> -    }
> 
> These error messages 'query-cpu-model-expansion reply data was
> missing ...' were replaced inside qemuMonitorJSONParseCPUModelData
> by error messages of the type 'QMP command reply data was missing ...'.
> I see that this was done because the function is used in later patches
> with different QMP commands, hence this change to a more generic
> error message. Which is fine.
> 
> A suggestion here would be to pass an extra string 'qmp-command-name'
> to qemuMonitorJSONParseCPUModelData. Then these error messages
> can be tuned to show which QMP command failed to parse. This can be
> useful if this parse function is used multiple times with several QMP
> commands.
> 
> 
> 
> Thanks,
> 
> DHB
> 
> 
> 

Fair enough. This is an easy change.

Thanks for all of the review!

[...]