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