10:06 a.m.
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(a)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");