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

[John Ferlan]

[...]

>>> Ideally a prerequisite. If that's too much yak shaving for you,
>>> at least add a comment to the validate function saying picking a default
>>> model really does not belong in a validation function.
> 
> Well it really should not be there. I've tried to make sure that at
> least the top level functions get a const domain definition, so that
> nobody even thinks about doing this.
> 
> Obviously for members of the domain object this can't be enforced by the
> compiler. Only via code review.
> 
> 
> This is the prototype for the validator:
> 
> static int
> qemuDomainDefValidate(const virDomainDef *def,
>                       virCapsPtr caps ATTRIBUTE_UNUSED,
>                       void *opaque)
> 
> 
> Do NOT try to set stuff there. The functions are meant only to check
> config.
> 

Have no worries, the patches don't "set" the def->controller[n]->model
value for SCSI controllers that don't have one set... If they did there
would be a bunch of make check failures because a value would be set and
thus alter output in some xml file the xml2xml testing.

/me wondering if a make check rule should be made to ensure that const
never changes...

>>>
>>
>> As usual a seemingly simple request sends me down the rabbit hole.
>> Setting a default controller model in qemuDomainControllerDefPostParse
>> would require passing @qemuCaps - simple enough until one checks out the
>> caller qemuDomainDeviceDefPostParse which notes:
>>
>> "
>> /* Note that qemuCaps may be NULL when this function is called. This
>>  * function shall not fail in that case. It will be re-run on VM startup
>>  * with the capabilities populated. */
> 
> I think I've seen this explained recently somewhere but I can reiterate.
> 
> The above statement applies to cases when the postparse callback is
> called on code paths which should not fail. This includes reloading
> configs from disks and a few other things.
> 
> qemuCaps will be NULL if the emulator binary is not present. We can't do
> much in that case, but we certainly should not drop domains.

Right, but if we cannot be sure that post parse would set the default,
then I would think paths that possibly rely on getting a caps based
model value cannot assume that the model was set during post parse. IOW:
They'd have to adjust too - as seen with the USB and Net model code paths.

> 
> Defining new domains still requires qemuCaps to be populated so that
> defaults can be loaded according to the qemu capabilities. In cases when
> qemuCaps are NULL most of the defaults should be present. In cases when
> we would do this during daemon upgrade, post parse will be re-run during
> VM startup when qemuCaps are required to be present anyways.
> 

Does today's truth hold true to some future? The API mentions that
qemuCaps "may" be NULL, but provides no real guidance over what
assumptions can be made by not setting something based on not having the
capabilities existing.

In any case, I'm working through some patches - let's see which rabbit
hole I get stuck in.

John