On 05/10/2016 07:15 AM, John Ferlan wrote:
On 05/10/2016 03:30 AM, Ján Tomko wrote:
On Mon, May 09, 2016 at 11:57:20AM -0400, Cole Robinson wrote:
On 05/09/2016 04:59 AM, Peter Krempa wrote:
On Fri, May 06, 2016 at 11:19:05 -0400, John Ferlan wrote:
On 05/02/2016 10:32 AM, Peter Krempa wrote: proper even though we hadn't rejected such a config when the "mode='host'" was first implemented.
Only when introducing a feature you are allowed to do a check that rejects parsing XML, afterwards, no such thins should be added.
Right, these rules make technical sense, but are extremely difficult to audit, and have proven hard to enforce. People wandering into the code may follow the conventions of the code around them, and have no idea that adding a new validation check is 'bad' when the pre-existing similar checks are 'good' strictly based one when they were added.
I think we need a better framework for this. I'll probably send a larger mail at some point, but basically I think we should:
- rename VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS to something generic like VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATION. The original flag is used at libvirtd startup time to avoid the 'disappearing domain' problem for when a qemu is uninstalled for example, but we still get that validation check for normal runtime XML define
The problem with skipping validation is that we will end up with invalid domains being defined, breaking assumptions in other parts of libvirt. That way we would have to duplicate the checks on domain startup too (which would not be such a problem if they were all in the same function).
For example: commit 21b316f4d351859d9ccbf8a20199f7e8707fd51d qemu: error out on missing machine type in configs which I added after someone tried to put the machine XML directly in /etc/libvirt.
There's no point in allowing the user to (try to) start such a domain, but currently we treat such unvalidated XML the same as XML from a fresh define.
Maybe some new API virIsDomainRunable (or some better name) that could be generated by carefully extracting checks from the qemu_command line build processing in order to make all those run/build time checks. Then command line building just fails if there's some sort of problem with the actual building rather than the config. Some of those checks are very specific configuration checks for supportable cross device or architecture validation. We don't provide an easy way for our consumers to validate something is good or bad until run time when it's rejected. For some consumers, that's a bad time to make that decision.
We already have qemuBuildCommandLineValidate which has a lot of those checks already. Moving validation out of the cli builder into that function has a bunch of benefits: - Like you suggest, we can re-use that function for validation at different points if we want - Simplifies the command line building code, which is the more interesting stuff - Will make eventual code coverage analysis easier. We don't need to worry too much about covering every validation check, but we _definitely_ want to cover every bit of the cli building. It'll be easier to spot the uncovered lines if we don't have validation mixed in
I think I recall an attempt to introudce an 'invalid' state, where you could not start the domain, but it was editable by libvirt APIs. Unfortunately I cannot find it in the archives. Does anyone remember what happened to it?
This one I believe is what you're looking for:
http://www.redhat.com/archives/libvir-list/2015-December/msg00027.html
Ooops, I just mailed the same link :) Should have read the whole thread first :) - Cole