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

[John Ferlan]

On 01/05/2018 10:20 AM, Marc Hartmayer wrote:
> On Thu, Jan 04, 2018 at 02:12 PM +0100, John Ferlan <jferlan at redhat.com> wrote:
>> On 01/04/2018 05:23 AM, Marc Hartmayer wrote:
>>> On Tue, Dec 12, 2017 at 04:06 PM +0100, John Ferlan <jferlan at redhat.com> 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?
> 

The split-up is done - there's a new series to include it as well as the
patch you posted...

Should make for hours of entertaining reading ;-)

John