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

John Ferlan jferlan at redhat.com
Mon Jan 29 21:18:46 UTC 2018



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

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. */
"

<sigh>

So, would adding the following to patch 1 for the Reset function suffice?


/** qemuDomainResetSCSIControllerModel
 * @def: Domain def
 * @qemuCaps: QEMU capabilities
 * @model: model to reset "default" to - either by @def or @qemuCaps
 *
 * Typically qemuDomainControllerDefPostParse would be used to set
 * a default controller model; however, qemuDomainDeviceDefPostParse
 * can be passed a NULL @qemuCaps, so setting the default model may
 * not be possible. Thus we're left to calling this Reset function
 * from numerous locations that need to get the default model for
 * the controller when one is not defined.
 *
 * Returns 0 on success, -1 on failure
 */

I can also add a note in qemuDomainDeviceDefValidateController that
restates this for this patch, e.g.:

if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
    /* Resetting the default should have been handled during post parse
     * parse callback; however, at that time we could not guarantee that
     * qemuCaps was valid, so we're left doing this now */
    if ((qemuDomainResetSCSIControllerModel(def, qemuCaps, &model)) < 0)
        return -1;
}

>> Are there drawbacks to doing so?  One drawback that I kept coming back
>> to is that LSI becomes the default qemu model instead of VIRTIO-SCSI
>> (it's the order of setting *model in qemuDomainSetSCSIControllerModel).
> 
> When was virtio-scsi the default model? Now we'd just record the
> picked default in XML.

virtio-scsi was never the default AFAIK... That wasn't the
point/problem. The problem is described in the next paragraph.

> 
> Jan
> 
>> That drawback caused problems when setting the model if we create a new
>> SCSI controller during something like hotplug because we had already
>> filled the current controller or when adding one VIRTIO-SCSI controller
>> but more than 7 disks (or hostdevs) that end up using that controller.
>> Although I think that's at least partially resolved by other changes
>> I've already made.

I looked it up, this is handled by c52dbafe and 07beea6c

John




More information about the libvir-list mailing list