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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
