[libvirt] [PATCH v3 05/12] qemu: Introduce qemuDomainDeviceDefValidateControllerSCSI

[John Ferlan]

On 12/08/2017 10:37 AM, Ján Tomko wrote:
> On Wed, Dec 06, 2017 at 08:14:01PM -0500, John Ferlan wrote:
>> Move SCSI validation from qemu_command into qemu_domain.
>> This includes the @model reset/check when the controller
>> model hasn't yet been set. While at it modify the switch
>> to account for all virDomainControllerModelSCSI types
>> rather than using the default label.
> 
> 'While at it' in the commit message is usually an indicator
> of a change that should have been in a separate commit.
> 

Sure... That's fine.  Couple more patches added...

>>
>> Rename/reorder the args in qemuCheckSCSIControllerIOThreads
>> to match the caller and also remove the unnecessary model
>> check as well as fixing up the comments to remove the previously
>> removed qemuCaps arg.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>> src/qemu/qemu_command.c | 94
>> +++++------------------------------------------
>> src/qemu/qemu_domain.c  | 97
>> ++++++++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 105 insertions(+), 86 deletions(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 2dd50a214..cdd267675 100644
> 
> [...]
> 
>> @@ -2662,47 +2616,17 @@ qemuBuildControllerDevStr(const virDomainDef
>> *domainDef,
>>
>>     *devstr = NULL;
>>
>> -    if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
>> -        if ((qemuDomainSetSCSIControllerModel(domainDef, qemuCaps,
>> &model)) < 0)
>> -            return -1;
>> -    }

Oh and of course I cannot remove this since this is called from hotplug
code we need to perhaps alter the model here...

>> -
>> -    if (!(def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
>> -          model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)) {
> 
> These errors are reported for non-SCSI controllers too.
> 

Hmm.. true, dang compound conditionals...

>> -        if (def->queues) {
>> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> -                           _("'queues' is only supported by
>> virtio-scsi controller"));
>> -            return -1;
>> -        }
>> -        if (def->cmd_per_lun) {
>> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> -                           _("'cmd_per_lun' is only supported by
>> virtio-scsi controller"));
>> -            return -1;
>> -        }
>> -        if (def->max_sectors) {
>> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> -                           _("'max_sectors' is only supported by
>> virtio-scsi controller"));
>> -            return -1;
>> -        }
>> -        if (def->ioeventfd) {
>> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> -                           _("'ioeventfd' is only supported by
>> virtio-scsi controller"));
>> -            return -1;
>> -        }
>> -    }
>> -
>>     switch ((virDomainControllerType) def->type) {
>>     case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
>> -        switch (model) {
>> +        if ((qemuDomainSetSCSIControllerModel(domainDef, qemuCaps,
>> &model)) < 0)
>> +            return -1;
> 
> After this patch, this function is called twice. Also it's called
> SetModel, while it's behaving as GetModel. Can we start assigning the
> default model in post-parse without breaking backwards compatibility?
> 
> Jan
> 

Are you asking that during PostParse callback for us to set def->model
so that we can just use it later on? That'd make things a lot easier
later on, but that seems a bit outside of what I was trying to do though.

Anyway based on this series and another I have posted... That means for
someone that didn't set the model, that a model would then be set and
written out... Meaning adjustment of a number of tests because now there
would be a model defined.  It also could mean we cannot adjust the
default model - one could say for example that using lsi as the default
model for scsi_host passthrough is perhaps less optimal than using
virtio-scsi... In fact, based on a different series and some testing
done - it may not work either.

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.

Tks -

John


>> +        switch ((virDomainControllerModelSCSI) model) {
>>         case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
>>             if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
>>                 virBufferAddLit(&buf, "virtio-scsi-ccw");
>> -                if (def->iothread) {
>> -                    if (!qemuCheckSCSIControllerIOThreads(domainDef,
>> def))
>> -                        goto error;
>> +                if (def->iothread)
>>                     virBufferAsprintf(&buf, ",iothread=iothread%u",
>>                                       def->iothread);
>> -                }
>>             } else if (def->info.type ==
>>                        VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) {
>>                 virBufferAddLit(&buf, "virtio-scsi-s390");