Re: [libvirt] [PATCH v4 01/13] qemu: Introduce qemuDomainDeviceDefValidateControllerAttributes

On Thu, Jan 04, 2018 at 02:12 PM +0100, John Ferlan <jferlan(a)redhat.com&gt; wrote: > On 01/04/2018 05:23 AM, Marc Hartmayer wrote: >> On Tue, Dec 12, 2017 at 04:06 PM +0100, John Ferlan <jferlan(a)redhat.com&gt; 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. >> […snip…] >>> virQEMUCapsPtr qemuCaps) >>> { >>> int ret = 0; >>> + int model = controller->model; >>> >>> if (!qemuDomainCheckCCWS390AddressSupport(def, controller->info, qemuCaps, >>> "controller")) >>> return -1; >>> >>> + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { >>> + if ((qemuDomainSetSCSIControllerModel(def, qemuCaps, &model)) < 0) >>> + return -1; >>> + } >> >> Didn't take a closer look, but is it the right place for this? (in a >> validation function) >> > > As noted above - this ends up being required in order to "get" the right > model type for SCSI because we don't set a default otherwise during post > parse. I'm somewhat conflicted over what to do and how much (more) > effort to make to this series which I originally thought would be a > simple move from one place to another, but now is taking on a different > direction/life <sigh>... Hmm - if you want to you can leave your patch at it is. It’s already much better than before. But I would definitely split 'qemuDomainSetSCSIControllerModel' into two separate functions as it does two distinct things. It either checks if the passed controller model is supported by the QEMU caps or it sets the controller model. “As for GetModel vs. SetModel - I think that's a different problem and can be put on a list of things to do after this is done.” Do you mean with your comment the same as I suggested above?