Re: [libvirt] [PATCH v5 02/16] qemu: Introduce qemuDomainDeviceDefValidateControllerAttributes
On Mon, Jan 29, 2018 at 16:18:46 -0500, John Ferlan wrote:
On 01/29/2018 12:53 PM, Ján Tomko wrote:
> On Mon, Jan 29, 2018 at 12:43:14PM -0500, John Ferlan wrote:
>>
>>
>> On 01/29/2018 04:18 AM, Ján Tomko wrote:
>>> On Fri, Jan 05, 2018 at 06:47:25PM -0500, 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.
>>>>
>>>
>>> So we should set it in post-parse processing once instead of adding this
>>> second call.
>>>
>>> Jan
>>
>> This is a point you raised in v3:
>>
>> https://www.redhat.com/archives/libvir-list/2017-December/msg00293.html
>>
>> to which I responded:
>>
>> https://www.redhat.com/archives/libvir-list/2017-December/msg00304.html
>>
>> and even carried into my v4:
>>
>> https://www.redhat.com/archives/libvir-list/2018-January/msg00103.html
>>
>> So, now that we're here again - the question becomes, do we really want
>> post parse processing to set a default model if it's not set?
>
> Yes, the used model should be recorded in the XML.
>
>> Should
>> that be part of this series? Should it be a followup? Should be
>> something done before this one and then rework this one?
>>
>
> 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.
>
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.
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.
Attachments:
- signature.asc (application/pgp-signature — 833 bytes)