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 ¯\_(ツ)_/¯ [...]
+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. [...]
@@ -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. -- Andrea Bolognani / Red Hat / Virtualization