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