[libvirt] [PATCH 8/8] Revert "conf: Validate disk lun using correct types"

[Cole Robinson]

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