[libvirt] [PATCH v2 03/25] qemu: Move <memballoon> validation out of qemu_command.c

[Cole Robinson]

On 01/24/2019 10:46 AM, Andrea Bolognani wrote:
> On Wed, 2019-01-23 at 16:32 -0500, Cole Robinson wrote:
> [...]
>> +static int
>> +qemuDomainDeviceDefValidateMemballoon(const virDomainMemballoonDef *memballoon,
> 
> You could pass
> 
>   const virDomainDef *def
> 
> too here, as most other qemuDomainDeviceDefValidate*() functions
> already do: that would allow you to...
> 
>> +                                      virQEMUCapsPtr qemuCaps)
>> +{
>> +    if (!memballoon ||
>> +        memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) {
>> +        return 0;
>> +    }
> 
> ... replace this with
> 
>   if (!virDomainDefHasMemballoon(def))
>       return 0;
> 
> which is arguably slightly nicer. But this version works perfectly
> fine, so it's entirely up to you whether to do that or not.
> 

I agree that would be nicer, but I dug a bit deeper: This code path is
triggered in two major different ways: as a part of validating a full
DomainDef, but also for validating an _individual_ device which hasn't
been added to a DomainDef yet. The latter path is via
virDomainDeviceDefParse which is called in hotplug situations. So to be
completely correct this function can't validate against def->memballoon
because it may not have been set yet.

> [...]
>> @@ -2463,11 +2462,10 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period,
>>      }
>>  
>>      if (persistentDef) {
>> -        if (!persistentDef->memballoon ||
>> -            persistentDef->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
>> +        if (!virDomainDefHasMemballoon(def)) {
> 
> s/def/persistentDef/
> 
> With this fixed,
> 



>   Reviewed-by: Andrea Bolognani <abologna at redhat.com>
> 

Nice catch

Thanks,
Cole