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

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.