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@xxxxxxxxxx>
>> ---
>> 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");
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
