[libvirt] [PATCH 0/2] conf: Tweaks to VirtIO SCSI controllers

Patch 2/2 implement a change that was suggested during the review process for the recent virtio-(non-)transitional work but was not considered as blocking for getting the code in, whereas patch 1/2 performs a trivial and hopefully entirely non-controversial enum member rename. Andrea Bolognani (2): conf: Rename VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI conf: Accept model=virtio for SCSI controllers docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 18 +++++++++++------- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 6 +++--- src/qemu/qemu_domain_address.c | 6 +++--- src/qemu/qemu_process.c | 2 +- src/vbox/vbox_common.c | 2 +- src/vz/vz_utils.c | 4 ++-- .../controller-virtio-scsi.xml | 2 +- 10 files changed, 25 insertions(+), 20 deletions(-) -- 2.20.1

While the string associated with the enum value is virtio-scsi, having the string "SCSI" show up twice in the name of the enum value is unnecessary and needlessly deviates from the by now well established practice of having MODEL_(VIRTIO|VIRTIO_TRANSITIONAL|VIRTIO_NON_TRANSITIONAL) enum values for VirtIO devices. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 6 +++--- src/qemu/qemu_domain_address.c | 6 +++--- src/qemu/qemu_process.c | 2 +- src/vbox/vbox_common.c | 2 +- src/vz/vz_utils.c | 4 ++-- 8 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d84cc2d482..0c0e422889 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4938,7 +4938,7 @@ static int virDomainControllerDefPostParse(virDomainControllerDefPtr cdev) { if (cdev->iothread && - cdev->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI && + cdev->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO && cdev->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL && cdev->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL) { virReportError(VIR_ERR_XML_ERROR, "%s", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c2dcc87ba1..860014d955 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -754,8 +754,8 @@ typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI, - VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078, + VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 853b0932b0..dee564905c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2999,7 +2999,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, switch ((virDomainControllerType)def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: switch ((virDomainControllerModelSCSI) def->model) { - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL: if (qemuBuildVirtioDevStr(&buf, "virtio-scsi", qemuCaps, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1487268a89..75307327a2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5087,7 +5087,7 @@ static int qemuDomainDeviceDefValidateControllerAttributes(const virDomainControllerDef *controller) { if (!(controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI && - (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI || + (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO || controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL || controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL))) { if (controller->queues) { @@ -5142,7 +5142,7 @@ qemuDomainCheckSCSIControllerModel(virQEMUCapsPtr qemuCaps, return false; } break; - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI)) { @@ -5259,7 +5259,7 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll const virDomainDef *def) { switch ((virDomainControllerModelSCSI) controller->model) { - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL: if (!qemuDomainCheckSCSIControllerIOThreads(controller, def)) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 4740536d82..05d059582e 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -58,11 +58,11 @@ qemuDomainGetSCSIControllerModel(const virDomainDef *def, if (qemuDomainIsPSeries(def)) return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI; else if (ARCH_IS_S390(def->os.arch)) - return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI; + return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO; else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI)) return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC; else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI)) - return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI; + return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO; virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to determine model for SCSI controller idx=%d"), @@ -647,7 +647,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT: return 0; - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL: return virtioFlags; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 68c670d3f2..99ed47bb2a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5193,7 +5193,7 @@ qemuProcessStartValidateIOThreads(virDomainObjPtr vm, virDomainControllerDefPtr cont = vm->def->controllers[i]; if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI && - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI && + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO && cont->iothread > 0 && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI_IOTHREAD)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 0a27deeaf8..9d68cd7119 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -405,7 +405,7 @@ vboxSetStorageController(virDomainControllerDefPtr controller, break; 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_VIRTIO: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078: diff --git a/src/vz/vz_utils.c b/src/vz/vz_utils.c index 13f5deeaa5..7766d341f2 100644 --- a/src/vz/vz_utils.c +++ b/src/vz/vz_utils.c @@ -144,7 +144,7 @@ vzInitCaps(unsigned long vzVersion, vzCapabilitiesPtr vzCaps) vzCaps->vmDiskFormat = VIR_STORAGE_FILE_QCOW2; vzCaps->diskBuses = vz7DiskBuses; vzCaps->controllerTypes = vz7ControllerTypes; - vzCaps->scsiControllerModel = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI; + vzCaps->scsiControllerModel = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO; } } @@ -471,7 +471,7 @@ int vzGetDefaultSCSIModel(vzDriverPtr driver, PRL_CLUSTERED_DEVICE_SUBTYPE *scsiModel) { switch ((int)driver->vzCaps.scsiControllerModel) { - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO: *scsiModel = PCD_VIRTIO_SCSI; break; case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC: -- 2.20.1

On Tue, Mar 05, 2019 at 04:07:39PM +0100, Andrea Bolognani wrote:
While the string associated with the enum value is virtio-scsi, having the string "SCSI" show up twice in the name of the enum value is unnecessary and needlessly deviates from the by now
It is not needless, it is for consistency with the already established model name which is 'virtio-scsi', not 'virtio'. Jano
well established practice of having
MODEL_(VIRTIO|VIRTIO_TRANSITIONAL|VIRTIO_NON_TRANSITIONAL)
enum values for VirtIO devices.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 6 +++--- src/qemu/qemu_domain_address.c | 6 +++--- src/qemu/qemu_process.c | 2 +- src/vbox/vbox_common.c | 2 +- src/vz/vz_utils.c | 4 ++-- 8 files changed, 13 insertions(+), 13 deletions(-)

At the moment, all VirtIO devices support model=virtio except for SCSI controllers where model=virtio-scsi must be used instead: get rid of this inconsistency by providing an alias at the parser level, so that existing code keeps working but using the same values across the board is also optionally possible. Tweak one of the test cases to show that the new value is accepted transparently, without altering any of the output files in the process. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 16 ++++++++++------ .../qemuxml2argvdata/controller-virtio-scsi.xml | 2 +- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 80f9f84f70..8981eda4b7 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2156,6 +2156,7 @@ <value>ibmvscsi</value> <value>virtio-scsi</value> <value>lsisas1078</value> + <value>virtio</value> <value>virtio-transitional</value> <value>virtio-non-transitional</value> </choice> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0c0e422889..cde10598f9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10320,16 +10320,20 @@ static int virDomainControllerModelTypeFromString(const virDomainControllerDef *def, const char *model) { - if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) - return virDomainControllerModelSCSITypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { + if (STREQ_NULLABLE(model, "virtio")) + return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO; + else + return virDomainControllerModelSCSITypeFromString(model); + } else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) { return virDomainControllerModelUSBTypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) + } else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { return virDomainControllerModelPCITypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) + } else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) { return virDomainControllerModelIDETypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) + } else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) { return virDomainControllerModelVirtioSerialTypeFromString(model); + } return -1; } diff --git a/tests/qemuxml2argvdata/controller-virtio-scsi.xml b/tests/qemuxml2argvdata/controller-virtio-scsi.xml index 844e4ad397..861121821c 100644 --- a/tests/qemuxml2argvdata/controller-virtio-scsi.xml +++ b/tests/qemuxml2argvdata/controller-virtio-scsi.xml @@ -47,7 +47,7 @@ <controller type='scsi' index='2' model='virtio-scsi'> <driver cmd_per_lun='50'/> </controller> - <controller type='scsi' index='3' model='virtio-scsi'> + <controller type='scsi' index='3' model='virtio'> <driver max_sectors='512'/> </controller> <controller type='scsi' index='4' model='virtio-scsi'> -- 2.20.1

On Tue, Mar 05, 2019 at 04:07:40PM +0100, Andrea Bolognani wrote:
At the moment, all VirtIO devices support model=virtio except for SCSI controllers where model=virtio-scsi must be used instead: get rid of this inconsistency by providing an alias at the parser level, so that existing code keeps working but using the same values across the board is also optionally possible.
Tweak one of the test cases to show that the new value is accepted transparently, without altering any of the output files in the process.
As in previous discussions of a similar type, I'm against adding multiple names for the 100% functionally identical devices. Previous naming choices from the past might not have been the best in retrospect, but that's not a reason to make future XML docs incompatible with existing libvirt by adding a 2nd different name for something that's alread suppprted. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Andrea Bolognani
-
Daniel P. Berrangé
-
Ján Tomko