Re: [libvirt] [PATCH 8/8] Revert "conf: Validate disk lun using correct types"
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