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.
--
Andrea Bolognani / Red Hat / Virtualization