[libvirt] [PATCH v1 00/26] move qemucaps validations from qemu_command to qemu_domain

[Cole Robinson]

On 12/9/19 6:15 PM, Daniel Henrique Barboza wrote:
> (series based on master commit 97cafa610ecf5)
> 
> This work was proposed by Cole in [1]. This is Cole's reasoning for
> it, copy/pasted from [1]:
> 
> -------
> The benefits of moving to validate time is that XML is rejected by
> 'virsh define' rather than at 'virsh start' time. It also makes it easier
> to follow the cli building code, and makes it easier to verify qemu_command.c
> test suite code coverage for the important stuff like covering every CLI
> option. It's also a good intermediate step for sharing validation with
> domain capabilities building, like is done presently for rng models.
> -------
> 
> Cole also mentioned that the machine features validation was a good
> place to start. I took it a step further and did it across the
> board on all qemu_command.c.
> 
> This series didn't create any new validation, just moved them
> to domain define time. Any other outcome is unintended.
> 
> 
> Not all cases where covered in this work. For a first version
> I decided to do only the most trivial cases. These are the
> other cases I left out for another day:
> 

Yes I think that's the right approach, handle the easy bits first and
deal the complicated bits in their own series. Some cases will likely
cause a lot of test suite churn, some cases may not even be worth it, we
will see!

> - all verifications contained in functions that are also called
> by qemu_hotplug.c. This means that the hotplug process will also
> use the validation code, and we can't just move it to qemu_domain.c.
> Besides, not all validation done in domain define time applies
> to hotplug, so it's not simply a matter of calling the same function
> from qemu_domain in qemu_hotplug. I have patches that moved the
> verification of all virtio options to qemu_domain.c, while also
> considering qemu_hotplug validation. In the end I decided to leave
> it away from this work for now because (1) it took 8 patches just
> for virtio case alone and (2) there are a lot of these cases in
> qemu_command.c and it would be too much to do it all at in this
> same series.
> 

The hotplug code should already be calling into Validate code. When a
device is hotplugged via qemu_driver, we get:

qemu_driver.c
  -> qemuDomainAttachDeviceFlags
   -> qemuDomainAttachDeviceLiveAndConfig
     -> virDomainDeviceDefParse
       -> virDomainDeviceDefValidate
         -> deviceValidateCallback
           -> qemuDomainDeviceDefValidate

So if device validation is moved into the correct
qemuDomainDeviceDefValidateXXX function it will get called in the
hotplug path so they won't be lost.

> - moving CPU Model validation is trickier than the rest because
> there are code in DefPostParse() that makes CPU Model changes that
> are then validated in qemu_command.c. Moving the validation to define
> time doesn't cut in this case - the validation is considering
> PostParse changes in the CPU Model and some of the will fail if
> done by qemuDomainDefValidate time. I didn't think too much about
> how to handle this case, but given that the approach would be
> different from the other cases handled here, I left it out too.
> 

Hmm I glanced at the qemuBuildCpuModelArgStr checks but it doesn't
strike me why those are an issue. Validate should always be triggered
after PostParse, so the ordering of those two shouldn't impact things
here. But I didn't attempt the change

> - SHMem: part of SHMem validation is being used by hotplug code.
> I could've moved the non-hotplug validation to define time,
> instead I decided it's better to to leave it to do it all at
> once in the future.
> 

shmem is just another device, so it should be fine as long as its final
location is triggered by qemuDomainDeviceDefValidate

> - DBus-VMState: validation of this fella must be done by first
> querying if the hash vm->priv->dbusVMState has elements.
> qemuDomainDefValidate does not have 'vm->priv' in it's API,
> meaning that I would need to either change the API to include
> it (which means changing domainValidateCallback), or do the
> validation by another branch where I can get access to vm->priv.
> 

Yeah this one is a little convoluted. The code is using the vm->priv
check to determine 'the user requested external slirp', which is really
a factor of interface type='user' + a qemu.conf setting, which we _can_
check at validate time against qemuCaps. But it's minor so I suggest
just ignoring it

> - vmcoreinfo: there are no challenges in moving vmcoreinfo
> validation to qemuDomainDefValidate. The problem were the
> unit tests. Moving this validation to domain define time
> breaks 925 tests on qemuxml2xmltest.c, including tests labelled
> as 'minimal' which should pass under any circurstances, as
> far as I understand. It is possible to just shoehorn
> QEMU_CAPS_DEVICE_VMCOREINFO in the qemuCaps of all tests, but
> this would tamper the idea of having to deal with NONE or
> LATEST or a specific set of capabilities for certain tests.
> Another case I would rather discuss separately.
> 

I suspect a bug on your side. qemu_command.c has:

static int

qemuBuildVMCoreInfoCommandLine(virCommandPtr cmd,

                               const virDomainDef *def,

                               virQEMUCapsPtr qemuCaps)

{

    virTristateSwitch vmci =
def->features[VIR_DOMAIN_FEATURE_VMCOREINFO];


    if (vmci != VIR_TRISTATE_SWITCH_ON)

        return 0;



    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMCOREINFO)) {

        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",

                       _("vmcoreinfo is not available "

                         "with this QEMU binary"));

        return -1;

    }



    virCommandAddArgList(cmd, "-device", "vmcoreinfo", NULL);

    return 0;

}

If you just copy the middle section, you'll get tons of failures. But
what you want to add is:

    if (def->features[VIR_DOMAIN_FEATURE_VMCOREINFO] ==
VIR_TRISTATE_SWITCH_ON &&
        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMCOREINFO))
        <error>

Thanks,
Cole