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

[Andrea Bolognani]

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