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

[Marc Hartmayer]

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?

>
> John
>
>>> +
>>> +    if (qemuDomainDeviceDefValidateControllerAttributes(controller, model) < 0)
>>> +        return -1;
>>> +
>>>      switch ((virDomainControllerType) controller->type) {
>>>      case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
>>>          ret = qemuDomainDeviceDefValidateControllerIDE(controller, def);
>>> --
>>> 2.13.6
>>>
>>> --
>>> libvir-list mailing list
>>> libvir-list at redhat.com
>>> https://www.redhat.com/mailman/listinfo/libvir-list
>>>
>> --
>> Beste Grüße / Kind regards
>>    Marc Hartmayer
>>
>> IBM Deutschland Research & Development GmbH
>> Vorsitzende des Aufsichtsrats: Martina Koederitz
>> Geschäftsführung: Dirk Wittkopp
>> Sitz der Gesellschaft: Böblingen
>> Registergericht: Amtsgericht Stuttgart, HRB 243294
>>
>
--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294