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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
