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

[Cole Robinson]

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:
>>> This reverts commit c79ebf53b5fe0a33bf407b3bcb49e3a27ec97eb4.
>>>
>>> We can't just add checks to the XML parser once we've accepted such
>>> configuration in the past.
>>> ---
>>>  src/conf/domain_conf.c   | 22 ----------------------
>>>  tests/qemuxml2argvtest.c |  3 +--
>>>  2 files changed, 1 insertion(+), 24 deletions(-)
>>>
>>
>> There was a bz associated with that commit - that'll need to be
>> addressed in some manner...
> 
> Well, the initial assesment of that BZ was wrong. This should have been
> fixed in virt manager at that point.
> 

That bz: https://bugzilla.redhat.com/show_bug.cgi?id=1201143

Is loaded with GSS and other 'priority' tags. Maybe the original assessment of
the bug is wrong. But just reverting the commit with little mention of that
until John dug it up is not helpful IMO

>> While I understand your point here, the configuration didn't work - that
>> is it couldn't be started anyway so there could not be a domain running
>> with that configuration and thus it wouldn't disappear on a subsequent
>> reload, hence why checking the config and rejecting "earlier" seemed
> 
> It does not kill any running domain, that's right. Defined domains still
> vanish after that commit if they were defined before. That is still
> unwanted.
> 

Yes, this is problematic, but then again has this issue bitten us in the year
since this was committed? We should still fix it, but it's not time
critical... Maybe we come up with a better solution.

>> 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

- Make an explicit virDomainDefValidate function and move the OSTYPE
validation there.

- Only run virDomainDefValidate if VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATION is
_not_ set. So skip validation at libvirtd startup

- Then for example we can move this block of code to that function

Doing all that, at least to cover this particular check, shouldn't be much
work, won't potentially revive the bug, _and_ it sets us up some nice
infrastructure to avoid this type of problem in the future.

Thanks,
Cole