[libvirt] [PATCH v5 02/16] qemu: Introduce qemuDomainDeviceDefValidateControllerAttributes

[John Ferlan]

On 01/29/2018 04:28 AM, Ján Tomko wrote:
> On Sun, Jan 28, 2018 at 09:48:19AM +0100, Michal Privoznik wrote:
>> On 01/06/2018 12:47 AM, John Ferlan wrote:
>>> Move the checks that various attributes are not set on any controller
>>> other than SCSI controller using virtio-scsi model into the common
>>> controller validate checks.
>>>
>>> Need to also add a qemuDomainResetSCSIControllerModel call in order
>>> to ensure we get the "right" SCSI model if it's not set by default
>>> since it wouldn't be set during post parse processing.
>>>
>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>> ---
>>>  src/qemu/qemu_command.c | 24 ------------------------
>>>  src/qemu/qemu_domain.c  | 41 +++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 41 insertions(+), 24 deletions(-)
>>
>>
>> The only problem I have with this approach is that while previously
>> we've checked for QEMU caps at domain start time, now we check for them
>> at define time. So I guess in general it's not a safe thing to do.
>>
>> For instance, I'd be against moving all checks done at cmd line time to
>> DefPostParse as they introduce TOCTOU problem.
> 
> This is not DefPostParse, this is DefValidate.
> 
> PostParse is called on all XML parsing (and as of <7c5cf4983> allowed
> to fail and then re-run on domain startup).
> 
> Validate is run when defining some new domains (the _VALIDATE flag has
> to be added to the APIs) and then unconditionally on domain startup.
> 

While I didn't go chase the commit for validate to be enabled, this is
essentially my understanding of validate too. Callbacks that while could
be in post parse callbacks, we don't put there because we don't want
running domains to disappear.  The whole post parse and validate and
flags can be really confusing....

> So the only problem here would be that we might not allow to define
> a domain until you install QEMU with the requested features.
> 

True, but I don't believe it wouldn't be a running domain that would
disappear as opposed to if these were post parse callback checks.

Conversely this may "help" in the detection of some invalid
configuration on a future change and test at XML2XML processing time
rather than XML2ARGV (and don't really run the domain) time.

>> However, some checks
>> (mostly semantic ones) can be done in post parse callbacks. For example,
>> trying to plug a disk onto ISA bus will fail regardless of qemu caps.
>> However, whether qemu supports VIRTIO_SCSI or not should not matter at
>> define time as this might change after domain is defined.
>>
>> However, SCSI controllers have been around for quite some time, so
>> unless somebody is upgrading from ancient qemu, we are safe.
>>
> 
> Generally we try not to break parsing existing configs (even with
> unusable domains), which is why Validate functions are separate
> from PostParse.
> 
> Jan
> 
>> ACK
>>
>> Michal


Having the ACK and an open question/issue is making me "pause"...

I'd like to resolve Jan's other point as well, but I'll respond to it
separately.

John