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@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");