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

[Cole Robinson]

offlist. Wow this looks awesome, thanks for tackling it! I'll give the
cover letter a response tomorrow when I've had time to digest it

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:
> 
> - 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.
> 
> - 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.
> 
> - 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.
> 
> - 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.
> 
> - 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.
> 
> 
> [1] https://www.redhat.com/archives/libvir-list/2019-October/msg00568.html
> 
> Daniel Henrique Barboza (26):
>   qemu_command.c: move PSeries features validation to qemu_domain.c
>   qemu_command.c: move mem.nosharepages validation to qemu_domain.c
>   qemu_command.c: move validation of vmport to qemu_domain.c
>   qemu_command.c: move nvdimm validation to qemu_domain.c
>   qemu_command.c: move I/O APIC validation to qemu_domain.c
>   numa_conf: add virDomainNumaNodesDistancesAreBeingSet() helper
>   qemu_command.c move NUMA validation to qemu_domain.c
>   qemu_command.c: move NVRAM validation to qemu_domain.c
>   qemu_command: move qemuBuildSoundDevStr caps validation to qemu_domain
>   qemu_command.c: move sound codec validation to qemu_domain.c
>   qemu_command: move qemuBuildHubDevStr caps validation to qemu_domain
>   qemu_command: move qemuBuildChrChardevStr caps validation to
>     qemu_domain
>   qemu: move qemuBuildHostdevCommandLine caps validation to qemu_domain
>   qemu_command.c: move vmGenID validation to qemu_domain.c
>   qemu: move qemuBuildSgaCommandLine validation to qemu_domain.c
>   qemu: move virDomainClockDef validation to qemu_domain.c
>   qemu: move qemuBuildPMCommandLine validation to qemu_domain.c
>   qemu: move qemuBuildBootCommandLine validation to qemu_domain.c
>   qemu_command.c: move pcihole64 validation to qemu_domain.c
>   qemu: move qemuBuildGraphicsSDLCommandLine validation to qemu_domain.c
>   qemu: move qemuBuildGraphicsVNCCommandLine validation to qemu_domain.c
>   qemu: move qemuBuildGraphicsSPICECommandLine validation to
>     qemu_domain.c
>   qemu: move qemuBuildGraphicsEGLHeadlessCommandLine validation to
>     qemu_domain.c
>   qemu: move qemuBuildSmartcardCommandLine validation to qemu_domain.c
>   qemu: move qemuBuildConsoleCommandLine validation to qemu_domain.c
>   qemu: move qemuBuildTPMDevStr TPM validation to qemu_domain.c
> 
>  src/conf/numa_conf.c                          |  19 +
>  src/conf/numa_conf.h                          |   2 +
>  src/libvirt_private.syms                      |   1 +
>  src/qemu/qemu_command.c                       | 575 +---------
>  src/qemu/qemu_command.h                       |   1 +
>  src/qemu/qemu_domain.c                        | 983 ++++++++++++++++--
>  tests/qemuhotplugtest.c                       |  10 +
>  tests/qemumemlocktest.c                       |  16 +-
>  .../default-video-type-aarch64.xml            |   2 +-
>  .../default-video-type-ppc64.xml              |   2 +-
>  .../default-video-type-s390x.xml              |   2 +-
>  .../graphics-egl-headless.args                |   2 +-
>  .../graphics-spice-egl-headless.args          |   2 +-
>  .../graphics-vnc-egl-headless.args            |   2 +-
>  tests/qemuxml2argvtest.c                      |  80 +-
>  ...ault-video-type-aarch64.aarch64-latest.xml |   4 +-
>  .../default-video-type-ppc64.ppc64-latest.xml |   4 +-
>  .../default-video-type-s390x.s390x-latest.xml |   4 +-
>  tests/qemuxml2xmltest.c                       | 244 +++--
>  19 files changed, 1246 insertions(+), 709 deletions(-)
> 


- Cole