[libvirt] [PATCH 11/18] qemu: Move memballoon validation out of command.c

[Cole Robinson]

On 01/21/2019 08:55 AM, Andrea Bolognani wrote:
> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
>> If we validate that memballoon is NONE or VIRTIO earlier,
>> we can simplify some checks in some driver APIs
> 
> Moving checks from the command line generation step to the domain
> validation step - that's what I'm talking about! \o/
> 
> [...]
>> +    if (STRPREFIX(def->os.machine, "s390-virtio") &&
>> +        virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390) && def->memballoon)
>> +        def->memballoon->model = VIR_DOMAIN_MEMBALLOON_MODEL_NONE;
> 
> This hunk of code makes zero sense to me, but that's what it's
> looked like until now and nobody cares about s390-virtio anyway, so
> I guess it doesn't matter ¯\_(ツ)_/¯
> 

Yeah it's a strange one for sure...

> [...]
>> +static int
>> +qemuDomainDeviceDefValidateMemballoon(const virDomainMemballoonDef *memballoon,
>> +                                      virQEMUCapsPtr qemuCaps)
>> +{
>> +    if (!memballoon ||
>> +        memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)
>> +        return 0;
> 
> This needs curly braces around the body, as per our coding style.
> 

Will do

> [...]
>> @@ -360,8 +360,7 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
>>              def->hostdevs[i]->info->type = type;
>>      }
>>  
>> -    if (def->memballoon &&
>> -        def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO &&
>> +    if (virDomainDefHasMemballoon(def) &&
>>          def->memballoon->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
>>          def->memballoon->info.type = type;
> 
> Again, I don't think you can get away with removing the model check
> here. As unlikely it is that we're ever going to get non-VirtIO
> memory balloons down the line, this new code doesn't look quite
> right to me, especially...
> 
> [...]
>> @@ -2436,8 +2436,7 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period,
>>      priv = vm->privateData;
>>  
>>      if (def) {
>> -        if (!def->memballoon ||
>> -            def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
>> +        if (!virDomainDefHasMemballoon(def)) {
>>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>                             _("Memory balloon model must be virtio to set the"
>>                               " collection period"));
> 
> ... if you look at the corresponding error messages.
> 
> I definitely like the change from checking def->memballoon to
> calling virDomainDefHasMemballoon(def), though.
> 

This discussion is a follow on from the previous one about rng... in
this case I don't think it's worth extending the VIRTIO whitelist check
in these two additional places, compared to just covering this at
Validate time. In this StatsPeriod case, I can change this error message to

"No memory balloon device configured, can not set the collection period"

Thanks,
Cole