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@redhat.com>
Nice catch Thanks, Cole