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