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