On 11/20/20 11:08 AM, Daniel P. Berrangé wrote:

The ESP SCSI controllers have same requirement as the LSI Logic controller for each disk to be set via the scsi-id=NNN property, not the lun=NNN property.

Maybe you could list the 3 controllers that are "ESP controllers" in the commit log message (it confused me for a minute because I didn't see the original commit where you added the support, so I didn't realize you'd added 3 separate new models, and not just a single new model) (I suspected it after seeing the code, of course, and a look with git blame verified my suspicion, so it was only momentary :-)

Switching the code to use an enum will force authors to pay attention to this difference when adding future SCSI controllers.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Laine Stump <laine@redhat.com> (with extra points for keeping even the thought of last-century hardware alive :-)

--- src/qemu/qemu_command.c | 26 +++++++++++++++++++++-- tests/qemuxml2argvdata/sparc-minimal.args | 4 ++-- 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index fbaacb8dd8..479bcc0b0c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1625,17 +1625,39 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, disk->info.addr.drive.controller))) return NULL;

- if (controllerModel == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC) { + switch ((virDomainControllerModelSCSI)controllerModel) { + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DC390: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AM53C974: virBufferAsprintf(&opt, ",bus=%s.%d,scsi-id=%d", contAlias, disk->info.addr.drive.bus, disk->info.addr.drive.unit); - } else { + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL: virBufferAsprintf(&opt, ",bus=%s.0,channel=%d,scsi-id=%d,lun=%d", contAlias, disk->info.addr.drive.bus, disk->info.addr.drive.target, disk->info.addr.drive.unit); + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected SCSI controller model %d"), + controllerModel); + return NULL; }

if (scsiVPDDeviceId) diff --git a/tests/qemuxml2argvdata/sparc-minimal.args b/tests/qemuxml2argvdata/sparc-minimal.args index 65cf99c895..0bf4c8f825 100644 --- a/tests/qemuxml2argvdata/sparc-minimal.args +++ b/tests/qemuxml2argvdata/sparc-minimal.args @@ -26,9 +26,9 @@ path=/tmp/lib/domain--1-redhat62sparc/monitor.sock,server,nowait \ -usb \ -drive file=/home/berrange/VirtualMachines/redhat-6.2-sparc.img,format=qcow2,\ if=none,id=drive-scsi0-0-0-0 \ --device scsi-hd,bus=scsi.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,\ +-device scsi-hd,bus=scsi.0,scsi-id=0,drive=drive-scsi0-0-0-0,\ id=scsi0-0-0-0,bootindex=1 \ -drive file=/home/berrange/VirtualMachines/redhat-6.2-sparc.iso,format=raw,\ if=none,id=drive-scsi0-0-0-1,readonly=on \ --device scsi-cd,bus=scsi.0,channel=0,scsi-id=0,lun=1,drive=drive-scsi0-0-0-1,\ +-device scsi-cd,bus=scsi.0,scsi-id=1,drive=drive-scsi0-0-0-1,\ id=scsi0-0-0-1