Add <controller type='scsi' model handling for virtio transitional
devices. Ex:
<controller type='scsi' model='virtio-transitional'/>
* "virtio-transitional" maps to qemu "virtio-scsi-pci-transitional"
* "virtio-non-transitional" maps to qemu "virtio-scsi-non-transitional"
The naming here doesn't match the pre-existing model=virtio-scsi.
The prescence of '-scsi' there seems kind of redundant as we have
type='scsi' already, so I decided to follow the pattern of other
patches and use virtio-transitional etc.
Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
docs/formatdomain.html.in | 4 ++--
docs/schemas/domaincommon.rng | 2 ++
src/conf/domain_conf.c | 4 +++-
src/conf/domain_conf.h | 2 ++
src/qemu/qemu_capabilities.c | 4 ++++
src/qemu/qemu_capabilities.h | 2 ++
src/qemu/qemu_command.c | 18 +++++++++++++--
src/qemu/qemu_domain.c | 8 ++++++-
src/qemu/qemu_domain_address.c | 2 ++
src/vbox/vbox_common.c | 2 ++
src/vmx/vmx.c | 4 +++-
.../caps_4.0.0.x86_64.xml | 2 ++
.../virtio-non-transitional.x86_64-3.1.0.args | 17 ++++++++------
...virtio-non-transitional.x86_64-latest.args | 17 ++++++++------
.../virtio-non-transitional.xml | 1 +
.../virtio-transitional.x86_64-3.1.0.args | 13 ++++++-----
.../virtio-transitional.x86_64-latest.args | 13 ++++++-----
.../qemuxml2argvdata/virtio-transitional.xml | 1 +
.../virtio-non-transitional.xml | 22 +++++++++++++------
.../virtio-transitional.xml | 15 ++++++++-----
tests/qemuxml2xmltest.c | 6 +++--
21 files changed, 111 insertions(+), 48 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 63bdf2c86b..bdf533ba3e 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4139,8 +4139,8 @@
<dt><code>scsi</code></dt>
<dd>A <code>scsi</code> controller has an optional attribute
<code>model</code>, which is one of 'auto', 'buslogic', 'ibmvscsi',
- 'lsilogic', 'lsisas1068', 'lsisas1078', 'virtio-scsi' or
- 'vmpvscsi'.</dd>
+ 'lsilogic', 'lsisas1068', 'lsisas1078', 'virtio-scsi',
+ 'vmpvscsi', 'virtio-transitional', 'virtio-non-transitional'</dd>
<dt><code>usb</code></dt>
<dd>A <code>usb</code> controller has an optional attribute
<code>model</code>, which is one of "piix3-uhci", "piix4-uhci",
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 58c449ca80..242f237360 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2152,6 +2152,8 @@
<value>ibmvscsi</value>
<value>virtio-scsi</value>
<value>lsisas1078</value>
+ <value>virtio-transitional</value>
+ <value>virtio-non-transitional</value>
</choice>
</attribute>
</optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index cbba02929c..76955b6d78 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -359,7 +359,9 @@ VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAS
"vmpvscsi",
"ibmvscsi",
"virtio-scsi",
- "lsisas1078");
+ "lsisas1078",
+ "virtio-transitional",
+ "virtio-non-transitional");
VIR_ENUM_IMPL(virDomainControllerModelUSB, VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST,
"piix3-uhci",
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index ded84f52d5..9771b0b3d7 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -756,6 +756,8 @@ typedef enum {
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_TRANSITIONAL,
+ VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL,
VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST
} virDomainControllerModelSCSI;
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index f5b9bc6937..0838f2a406 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -542,6 +542,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"vhost-vsock-pci-non-transitional",
"virtio-input-host-pci-transitional",
"virtio-input-host-pci-non-transitional",
+ "virtio-scsi-pci-transitional",
+ "virtio-scsi-pci-non-transitional",
);
@@ -1146,6 +1148,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
{"vhost-vsock-pci-non-transitional", QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL},
{"virtio-input-host-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL},
{"virtio-input-host-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL},
+ {"virtio-scsi-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL},
+ {"virtio-scsi-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL},
};
static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 91aae8df5b..d23bd251f2 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -526,6 +526,8 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL, /* -device vhost-vsock-pci-non-transitional */
QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL, /* -device virtio-input-host-pci-transitional */
QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL, /* -device virtio-input-host-pci-non-transitional */
+ QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL, /* -device virtio-scsi-pci-transitional */
+ QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL, /* -device virtio-scsi-pci-non-transitional */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4d14ac9e5d..50042b2919 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -507,12 +507,20 @@ qemuBuildVirtioTransitional(virBufferPtr buf,
tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL;
ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL;
break;
+ case VIR_DOMAIN_DEVICE_CONTROLLER:
+ if (strstr(baseName, "scsi")) {
+ has_tmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL;
+ has_ntmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL;
+ tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL;
+ ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL;
+ break;
+ }
+ return 0;
case VIR_DOMAIN_DEVICE_LEASE:
case VIR_DOMAIN_DEVICE_SOUND:
case VIR_DOMAIN_DEVICE_VIDEO:
case VIR_DOMAIN_DEVICE_WATCHDOG:
- case VIR_DOMAIN_DEVICE_CONTROLLER:
case VIR_DOMAIN_DEVICE_GRAPHICS:
case VIR_DOMAIN_DEVICE_HUB:
case VIR_DOMAIN_DEVICE_REDIRDEV:
@@ -2971,8 +2979,14 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
switch ((virDomainControllerModelSCSI) def->model) {
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
- if (qemuBuildVirtioDevStr(&buf, "virtio-scsi", def->info.type) < 0)
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL:
+ if (qemuBuildVirtioTransitional(&buf, "virtio-scsi", qemuCaps,
+ def->info.type,
+ def->model, NULL,
+ VIR_DOMAIN_DEVICE_CONTROLLER) < 0) {
goto error;
+ }
if (def->iothread) {
virBufferAsprintf(&buf, ",iothread=iothread%u",
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6d54727a31..85f7b6a4c8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4972,7 +4972,9 @@ 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_SCSI ||
+ controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL ||
+ controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL))) {
if (controller->queues) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("'queues' is only supported by virtio-scsi controller"));
@@ -5026,6 +5028,8 @@ qemuDomainCheckSCSIControllerModel(virQEMUCapsPtr qemuCaps,
}
break;
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL:
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("This QEMU doesn't support "
@@ -5141,6 +5145,8 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll
{
switch ((virDomainControllerModelSCSI) controller->model) {
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL:
if (!qemuDomainCheckSCSIControllerIOThreads(controller, def))
return -1;
break;
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 70591dce25..9d07fa133e 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -651,8 +651,10 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
return 0;
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL:
return virtioFlags;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL:
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 664650f217..00d43d9a83 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -406,6 +406,8 @@ vboxSetStorageController(virDomainControllerDefPtr controller,
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_TRANSITIONAL:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL:
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("The vbox driver does not support %s SCSI "
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 92601291fd..a853c05e86 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -517,7 +517,9 @@ VIR_ENUM_IMPL(virVMXControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST,
"pvscsi",
"UNUSED ibmvscsi",
"UNUSED virtio-scsi",
- "UNUSED lsisas1078");
+ "UNUSED lsisas1078",
+ "UNUSED virtio-transitional",
+ "UNUSED virtio-non-transitional");
/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
index d5bbbc40f7..aa9a0d2d3b 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
@@ -228,6 +228,8 @@
<flag name='vhost-vsock-pci-non-transitional'/>
<flag name='virtio-input-host-pci-transitional'/>
<flag name='virtio-input-host-pci-non-transitional'/>
+ <flag name='virtio-scsi-pci-transitional'/>
+ <flag name='virtio-scsi-pci-non-transitional'/>
<version>3001050</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>446361</microcodeVersion>
diff --git a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args
index fc183757b0..6fea53fa6e 100644
--- a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args
+++ b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args
@@ -32,9 +32,12 @@ addr=0x1 \
-device pcie-root-port,port=0xd,chassis=6,id=pci.6,bus=pcie.0,addr=0x1.0x5 \
-device pcie-root-port,port=0xe,chassis=7,id=pci.7,bus=pcie.0,addr=0x1.0x6 \
-device pcie-root-port,port=0xf,chassis=8,id=pci.8,bus=pcie.0,addr=0x1.0x7 \
--device pcie-root-port,port=0x10,chassis=9,id=pci.9,bus=pcie.0,addr=0x2 \
+-device pcie-root-port,port=0x10,chassis=9,id=pci.9,bus=pcie.0,\
+multifunction=on,addr=0x2 \
+-device pcie-root-port,port=0x11,chassis=10,id=pci.10,bus=pcie.0,addr=0x2.0x1 \
+-device virtio-scsi-pci,disable-legacy=on,id=scsi0,bus=pci.3,addr=0x0 \
-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \
--device virtio-blk-pci,disable-legacy=on,scsi=off,bus=pci.3,addr=0x0,\
+-device virtio-blk-pci,disable-legacy=on,scsi=off,bus=pci.4,addr=0x0,\
drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \
-fsdev local,security_model=passthrough,id=fsdev-fs0,path=/export/fs1 \
-device virtio-9p-pci,disable-legacy=on,id=fs0,fsdev=fsdev-fs0,mount_tag=fs1,\
@@ -43,15 +46,15 @@ bus=pci.1,addr=0x0 \
-device virtio-net-pci,disable-legacy=on,netdev=hostnet0,id=net0,\
mac=00:11:22:33:44:55,bus=pci.2,addr=0x0 \
-device virtio-input-host-pci,disable-legacy=on,id=input0,\
-evdev=/dev/input/event1234,bus=pci.7,addr=0x0 \
+evdev=/dev/input/event1234,bus=pci.8,addr=0x0 \
-device vhost-scsi-pci,disable-legacy=on,wwpn=naa.5123456789abcde0,vhostfd=3,\
-id=hostdev0,bus=pci.4,addr=0x0 \
--device virtio-balloon-pci,disable-legacy=on,id=balloon0,bus=pci.5,addr=0x0 \
+id=hostdev0,bus=pci.5,addr=0x0 \
+-device virtio-balloon-pci,disable-legacy=on,id=balloon0,bus=pci.6,addr=0x0 \
-object rng-random,id=objrng0,filename=/dev/urandom \
--device virtio-rng-pci,disable-legacy=on,rng=objrng0,id=rng0,bus=pci.6,\
+-device virtio-rng-pci,disable-legacy=on,rng=objrng0,id=rng0,bus=pci.7,\
addr=0x0 \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
resourcecontrol=deny \
-device vhost-vsock-pci,disable-legacy=on,id=vsock0,guest-cid=4,vhostfd=6789,\
-bus=pci.8,addr=0x0 \
+bus=pci.9,addr=0x0 \
-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args
index 383b29f629..ad644de35d 100644
--- a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args
@@ -32,9 +32,12 @@ addr=0x1 \
-device pcie-root-port,port=0xd,chassis=6,id=pci.6,bus=pcie.0,addr=0x1.0x5 \
-device pcie-root-port,port=0xe,chassis=7,id=pci.7,bus=pcie.0,addr=0x1.0x6 \
-device pcie-root-port,port=0xf,chassis=8,id=pci.8,bus=pcie.0,addr=0x1.0x7 \
--device pcie-root-port,port=0x10,chassis=9,id=pci.9,bus=pcie.0,addr=0x2 \
+-device pcie-root-port,port=0x10,chassis=9,id=pci.9,bus=pcie.0,\
+multifunction=on,addr=0x2 \
+-device pcie-root-port,port=0x11,chassis=10,id=pci.10,bus=pcie.0,addr=0x2.0x1 \
+-device virtio-scsi-pci-non-transitional,id=scsi0,bus=pci.3,addr=0x0 \
-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \
--device virtio-blk-pci-non-transitional,scsi=off,bus=pci.3,addr=0x0,\
+-device virtio-blk-pci-non-transitional,scsi=off,bus=pci.4,addr=0x0,\
drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \
-fsdev local,security_model=passthrough,id=fsdev-fs0,path=/export/fs1 \
-device virtio-9p-pci-non-transitional,id=fs0,fsdev=fsdev-fs0,mount_tag=fs1,\
@@ -43,14 +46,14 @@ bus=pci.1,addr=0x0 \
-device virtio-net-pci-non-transitional,netdev=hostnet0,id=net0,\
mac=00:11:22:33:44:55,bus=pci.2,addr=0x0 \
-device virtio-input-host-pci-non-transitional,id=input0,\
-evdev=/dev/input/event1234,bus=pci.7,addr=0x0 \
+evdev=/dev/input/event1234,bus=pci.8,addr=0x0 \
-device vhost-scsi-pci-non-transitional,wwpn=naa.5123456789abcde0,vhostfd=3,\
-id=hostdev0,bus=pci.4,addr=0x0 \
--device virtio-balloon-pci-non-transitional,id=balloon0,bus=pci.5,addr=0x0 \
+id=hostdev0,bus=pci.5,addr=0x0 \
+-device virtio-balloon-pci-non-transitional,id=balloon0,bus=pci.6,addr=0x0 \
-object rng-random,id=objrng0,filename=/dev/urandom \
--device virtio-rng-pci-non-transitional,rng=objrng0,id=rng0,bus=pci.6,addr=0x0 \
+-device virtio-rng-pci-non-transitional,rng=objrng0,id=rng0,bus=pci.7,addr=0x0 \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
resourcecontrol=deny \
-device vhost-vsock-pci-non-transitional,id=vsock0,guest-cid=4,vhostfd=6789,\
-bus=pci.8,addr=0x0 \
+bus=pci.9,addr=0x0 \
-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/virtio-non-transitional.xml b/tests/qemuxml2argvdata/virtio-non-transitional.xml
index 596aa1015b..03273dc2d9 100644
--- a/tests/qemuxml2argvdata/virtio-non-transitional.xml
+++ b/tests/qemuxml2argvdata/virtio-non-transitional.xml
@@ -29,6 +29,7 @@
<input type='passthrough' bus='virtio' model='virtio-non-transitional'>
<source evdev='/dev/input/event1234'/>
</input>
+ <controller type='scsi' model='virtio-non-transitional'/>
<controller type='usb' index='0' model='none'/>
<memballoon model='virtio-non-transitional'/>
<vsock model='virtio-non-transitional'>
diff --git a/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args b/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args
index 8046e5c102..e8b3712acb 100644
--- a/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args
+++ b/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args
@@ -27,8 +27,9 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
addr=0x1 \
-device pcie-pci-bridge,id=pci.2,bus=pci.1,addr=0x0 \
-device pcie-root-port,port=0x9,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x1 \
+-device virtio-scsi-pci,id=scsi0,bus=pci.2,addr=0x3 \
-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \
--device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x3,drive=drive-virtio-disk0,\
+-device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x4,drive=drive-virtio-disk0,\
id=virtio-disk0,bootindex=1 \
-fsdev local,security_model=passthrough,id=fsdev-fs0,path=/export/fs1 \
-device virtio-9p-pci,id=fs0,fsdev=fsdev-fs0,mount_tag=fs1,bus=pci.2,addr=0x1 \
@@ -36,13 +37,13 @@ id=virtio-disk0,bootindex=1 \
-device virtio-net-pci,netdev=hostnet0,id=net0,mac=00:11:22:33:44:55,bus=pci.2,\
addr=0x2 \
-device virtio-input-host-pci,id=input0,evdev=/dev/input/event1234,bus=pci.2,\
-addr=0x7 \
+addr=0x8 \
-device vhost-scsi-pci,wwpn=naa.5123456789abcde0,vhostfd=3,id=hostdev0,\
-bus=pci.2,addr=0x4 \
--device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x5 \
+bus=pci.2,addr=0x5 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x6 \
-object rng-random,id=objrng0,filename=/dev/urandom \
--device virtio-rng-pci,rng=objrng0,id=rng0,bus=pci.2,addr=0x6 \
+-device virtio-rng-pci,rng=objrng0,id=rng0,bus=pci.2,addr=0x7 \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
resourcecontrol=deny \
--device vhost-vsock-pci,id=vsock0,guest-cid=4,vhostfd=6789,bus=pci.2,addr=0x8 \
+-device vhost-vsock-pci,id=vsock0,guest-cid=4,vhostfd=6789,bus=pci.2,addr=0x9 \
-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args b/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args
index 410eb28f0a..95d20f8579 100644
--- a/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args
@@ -27,8 +27,9 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
addr=0x1 \
-device pcie-pci-bridge,id=pci.2,bus=pci.1,addr=0x0 \
-device pcie-root-port,port=0x9,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x1 \
+-device virtio-scsi-pci-transitional,id=scsi0,bus=pci.2,addr=0x3 \
-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \
--device virtio-blk-pci-transitional,scsi=off,bus=pci.2,addr=0x3,\
+-device virtio-blk-pci-transitional,scsi=off,bus=pci.2,addr=0x4,\
drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \
-fsdev local,security_model=passthrough,id=fsdev-fs0,path=/export/fs1 \
-device virtio-9p-pci-transitional,id=fs0,fsdev=fsdev-fs0,mount_tag=fs1,\
@@ -37,14 +38,14 @@ bus=pci.2,addr=0x1 \
-device virtio-net-pci-transitional,netdev=hostnet0,id=net0,\
mac=00:11:22:33:44:55,bus=pci.2,addr=0x2 \
-device virtio-input-host-pci-transitional,id=input0,\
-evdev=/dev/input/event1234,bus=pci.2,addr=0x7 \
+evdev=/dev/input/event1234,bus=pci.2,addr=0x8 \
-device vhost-scsi-pci-transitional,wwpn=naa.5123456789abcde0,vhostfd=3,\
-id=hostdev0,bus=pci.2,addr=0x4 \
--device virtio-balloon-pci-transitional,id=balloon0,bus=pci.2,addr=0x5 \
+id=hostdev0,bus=pci.2,addr=0x5 \
+-device virtio-balloon-pci-transitional,id=balloon0,bus=pci.2,addr=0x6 \
-object rng-random,id=objrng0,filename=/dev/urandom \
--device virtio-rng-pci-transitional,rng=objrng0,id=rng0,bus=pci.2,addr=0x6 \
+-device virtio-rng-pci-transitional,rng=objrng0,id=rng0,bus=pci.2,addr=0x7 \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
resourcecontrol=deny \
-device vhost-vsock-pci-transitional,id=vsock0,guest-cid=4,vhostfd=6789,\
-bus=pci.2,addr=0x8 \
+bus=pci.2,addr=0x9 \
-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/virtio-transitional.xml b/tests/qemuxml2argvdata/virtio-transitional.xml
index 90fba68d9f..1616cb137b 100644
--- a/tests/qemuxml2argvdata/virtio-transitional.xml
+++ b/tests/qemuxml2argvdata/virtio-transitional.xml
@@ -29,6 +29,7 @@
<input type='passthrough' bus='virtio' model='virtio-transitional'>
<source evdev='/dev/input/event1234'/>
</input>
+ <controller type='scsi' model='virtio-transitional'/>
<controller type='usb' index='0' model='none'/>
<memballoon model='virtio-transitional'/>
<vsock model='virtio-transitional'>
diff --git a/tests/qemuxml2xmloutdata/virtio-non-transitional.xml b/tests/qemuxml2xmloutdata/virtio-non-transitional.xml
index b6e762c0a7..56b673d7fa 100644
--- a/tests/qemuxml2xmloutdata/virtio-non-transitional.xml
+++ b/tests/qemuxml2xmloutdata/virtio-non-transitional.xml
@@ -18,8 +18,11 @@
<driver name='qemu' type='raw'/>
<source dev='/dev/HostVG/QEMUGuest1'/>
<target dev='vda' bus='virtio'/>
- <address type='pci' domain='0x0000' bus='0x03' slot='0x00' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x0'/>
</disk>
+ <controller type='scsi' index='0' model='virtio-non-transitional'>
+ <address type='pci' domain='0x0000' bus='0x03' slot='0x00' function='0x0'/>
+ </controller>
<controller type='usb' index='0' model='none'/>
<controller type='sata' index='0'>
<address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
@@ -68,7 +71,12 @@
<controller type='pci' index='9' model='pcie-root-port'>
<model name='pcie-root-port'/>
<target chassis='9' port='0x10'/>
- <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0' multifunction='on'/>
+ </controller>
+ <controller type='pci' index='10' model='pcie-root-port'>
+ <model name='pcie-root-port'/>
+ <target chassis='10' port='0x11'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x1'/>
</controller>
<filesystem type='mount' accessmode='passthrough' model='virtio-non-transitional'>
<source dir='/export/fs1'/>
@@ -82,24 +90,24 @@
</interface>
<input type='passthrough' bus='virtio' model='virtio-non-transitional'>
<source evdev='/dev/input/event1234'/>
- <address type='pci' domain='0x0000' bus='0x07' slot='0x00' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x08' slot='0x00' function='0x0'/>
</input>
<input type='mouse' bus='ps2'/>
<input type='keyboard' bus='ps2'/>
<hostdev mode='subsystem' type='scsi_host' managed='no' model='virtio-non-transitional'>
<source protocol='vhost' wwpn='naa.5123456789abcde0'/>
- <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x05' slot='0x00' function='0x0'/>
</hostdev>
<memballoon model='virtio-non-transitional'>
- <address type='pci' domain='0x0000' bus='0x05' slot='0x00' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x06' slot='0x00' function='0x0'/>
</memballoon>
<rng model='virtio-non-transitional'>
<backend model='random'>/dev/urandom</backend>
- <address type='pci' domain='0x0000' bus='0x06' slot='0x00' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x07' slot='0x00' function='0x0'/>
</rng>
<vsock model='virtio-non-transitional'>
<cid auto='no' address='4'/>
- <address type='pci' domain='0x0000' bus='0x08' slot='0x00' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x09' slot='0x00' function='0x0'/>
</vsock>
</devices>
</domain>
diff --git a/tests/qemuxml2xmloutdata/virtio-transitional.xml b/tests/qemuxml2xmloutdata/virtio-transitional.xml
index 9fa9732c2d..cf6c43bcb0 100644
--- a/tests/qemuxml2xmloutdata/virtio-transitional.xml
+++ b/tests/qemuxml2xmloutdata/virtio-transitional.xml
@@ -18,8 +18,11 @@
<driver name='qemu' type='raw'/>
<source dev='/dev/HostVG/QEMUGuest1'/>
<target dev='vda' bus='virtio'/>
- <address type='pci' domain='0x0000' bus='0x02' slot='0x03' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x02' slot='0x04' function='0x0'/>
</disk>
+ <controller type='scsi' index='0' model='virtio-transitional'>
+ <address type='pci' domain='0x0000' bus='0x02' slot='0x03' function='0x0'/>
+ </controller>
<controller type='usb' index='0' model='none'/>
<controller type='sata' index='0'>
<address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
@@ -51,24 +54,24 @@
</interface>
<input type='passthrough' bus='virtio' model='virtio-transitional'>
<source evdev='/dev/input/event1234'/>
- <address type='pci' domain='0x0000' bus='0x02' slot='0x07' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x02' slot='0x08' function='0x0'/>
</input>
<input type='mouse' bus='ps2'/>
<input type='keyboard' bus='ps2'/>
<hostdev mode='subsystem' type='scsi_host' managed='no' model='virtio-transitional'>
<source protocol='vhost' wwpn='naa.5123456789abcde0'/>
- <address type='pci' domain='0x0000' bus='0x02' slot='0x04' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x02' slot='0x05' function='0x0'/>
</hostdev>
<memballoon model='virtio-transitional'>
- <address type='pci' domain='0x0000' bus='0x02' slot='0x05' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x02' slot='0x06' function='0x0'/>
</memballoon>
<rng model='virtio-transitional'>
<backend model='random'>/dev/urandom</backend>
- <address type='pci' domain='0x0000' bus='0x02' slot='0x06' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x02' slot='0x07' function='0x0'/>
</rng>
<vsock model='virtio-transitional'>
<cid auto='no' address='4'/>
- <address type='pci' domain='0x0000' bus='0x02' slot='0x08' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x02' slot='0x09' function='0x0'/>
</vsock>
</devices>
</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 9fe2b8e8a2..114ceca16c 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -1271,14 +1271,16 @@ mymain(void)
QEMU_CAPS_DEVICE_PCIE_ROOT_PORT,
QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY,
QEMU_CAPS_DEVICE_VHOST_VSOCK,
- QEMU_CAPS_VIRTIO_INPUT_HOST);
+ QEMU_CAPS_VIRTIO_INPUT_HOST,
+ QEMU_CAPS_VIRTIO_SCSI);
DO_TEST("virtio-non-transitional",
QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE,
QEMU_CAPS_DEVICE_PCIE_ROOT_PORT,
QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY,
QEMU_CAPS_DEVICE_VHOST_VSOCK,
- QEMU_CAPS_VIRTIO_INPUT_HOST);
+ QEMU_CAPS_VIRTIO_INPUT_HOST,
+ QEMU_CAPS_VIRTIO_SCSI);
if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
virFileDeleteTree(fakerootdir);
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
> Add <controller type='scsi' model handling for virtio transitional
> devices. Ex:
>
> <controller type='scsi' model='virtio-transitional'/>
>
> * "virtio-transitional" maps to qemu "virtio-scsi-pci-transitional"
> * "virtio-non-transitional" maps to qemu "virtio-scsi-non-transitional"
>
> The naming here doesn't match the pre-existing model=virtio-scsi.
> The prescence of '-scsi' there seems kind of redundant as we have
> type='scsi' already, so I decided to follow the pattern of other
> patches and use virtio-transitional etc.
Completely agree with the rationale here; however, in order to
really match the way other devices are handled, I think we should
make it so you can use
<controller type='scsi' model='virtio'/>
as well, which of course would behave the same as the currently
available version. What do you think?
[...]
> +++ b/src/conf/domain_conf.c
> @@ -359,7 +359,9 @@ VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAS
> "vmpvscsi",
> "ibmvscsi",
> "virtio-scsi",
> - "lsisas1078");
> + "lsisas1078",
> + "virtio-transitional",
> + "virtio-non-transitional");
Same comment as always for VIR_ENUM_IMPL().
[...]
> @@ -1146,6 +1148,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
> {"vhost-vsock-pci-non-transitional", QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL},
> {"virtio-input-host-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL},
> {"virtio-input-host-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL},
> + {"virtio-scsi-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL},
> + {"virtio-scsi-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL},
> };
Same comment as always for capabilities.
[...]
> @@ -507,12 +507,20 @@ qemuBuildVirtioTransitional(virBufferPtr buf,
> tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL;
> ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL;
> break;
Empty line here.
> + case VIR_DOMAIN_DEVICE_CONTROLLER:
> + if (strstr(baseName, "scsi")) {
> + has_tmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL;
> + has_ntmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL;
> + tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL;
> + ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL;
> + break;
> + }
> + return 0;
Using strstr() here is kinda crude, especially since the caller has
enough information to pass the appropriate virDomainControllerType
value, both in this case and later on for serial controllers.
I'd say just add yet another argument to the function. Yeah, it
starts to get quite unsightly, but I'd really rather not resort to
string comparison when a nice, type safe enum will do.
[...]
> +++ b/tests/qemuxml2xmltest.c
> @@ -1271,14 +1271,16 @@ mymain(void)
> QEMU_CAPS_DEVICE_PCIE_ROOT_PORT,
> QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY,
> QEMU_CAPS_DEVICE_VHOST_VSOCK,
> - QEMU_CAPS_VIRTIO_INPUT_HOST);
> + QEMU_CAPS_VIRTIO_INPUT_HOST,
> + QEMU_CAPS_VIRTIO_SCSI);
> DO_TEST("virtio-non-transitional",
> QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
> QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE,
> QEMU_CAPS_DEVICE_PCIE_ROOT_PORT,
> QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY,
> QEMU_CAPS_DEVICE_VHOST_VSOCK,
> - QEMU_CAPS_VIRTIO_INPUT_HOST);
> + QEMU_CAPS_VIRTIO_INPUT_HOST,
> + QEMU_CAPS_VIRTIO_SCSI);
This too could go in patch 2/18.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 01/21/2019 11:49 AM, Andrea Bolognani wrote:
> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
>> Add <controller type='scsi' model handling for virtio transitional
>> devices. Ex:
>>
>> <controller type='scsi' model='virtio-transitional'/>
>>
>> * "virtio-transitional" maps to qemu "virtio-scsi-pci-transitional"
>> * "virtio-non-transitional" maps to qemu "virtio-scsi-non-transitional"
>>
>> The naming here doesn't match the pre-existing model=virtio-scsi.
>> The prescence of '-scsi' there seems kind of redundant as we have
>> type='scsi' already, so I decided to follow the pattern of other
>> patches and use virtio-transitional etc.
>
> Completely agree with the rationale here; however, in order to
> really match the way other devices are handled, I think we should
> make it so you can use
>
> <controller type='scsi' model='virtio'/>
>
> as well, which of course would behave the same as the currently
> available version. What do you think?
>
Yes I agree it would be nice to add that option. I suggest we make it a
separate issue from this series though incase it's a contentious idea,
v2 is already shaping up to be ~30 patches...
>
>> + case VIR_DOMAIN_DEVICE_CONTROLLER:
>> + if (strstr(baseName, "scsi")) {
>> + has_tmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL;
>> + has_ntmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL;
>> + tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL;
>> + ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL;
>> + break;
>> + }
>> + return 0;
>
> Using strstr() here is kinda crude, especially since the caller has
> enough information to pass the appropriate virDomainControllerType
> value, both in this case and later on for serial controllers.
>
> I'd say just add yet another argument to the function. Yeah, it
> starts to get quite unsightly, but I'd really rather not resort to
> string comparison when a nice, type safe enum will do.
>
Yeah it's hacky. Adding another arg here is going to add extra pain if
when this is merged into qemuBuildVirtioStr, most callers are going have
NULL arguments, and an increased chance of new future callers passing in
incorrect values and accidentally triggering a wrong code path. I feel
like this is another argument for the separated BuildTransitional or
whatever, but I'm not sold either way so I'll stick with your suggestion
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 01/22/2019 12:39 PM, Cole Robinson wrote:
> On 01/21/2019 11:49 AM, Andrea Bolognani wrote:
>> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
>>> Add <controller type='scsi' model handling for virtio transitional
>>> devices. Ex:
>>>
>>> <controller type='scsi' model='virtio-transitional'/>
>>>
>>> * "virtio-transitional" maps to qemu "virtio-scsi-pci-transitional"
>>> * "virtio-non-transitional" maps to qemu "virtio-scsi-non-transitional"
>>>
>>> The naming here doesn't match the pre-existing model=virtio-scsi.
>>> The prescence of '-scsi' there seems kind of redundant as we have
>>> type='scsi' already, so I decided to follow the pattern of other
>>> patches and use virtio-transitional etc.
>>
>> Completely agree with the rationale here; however, in order to
>> really match the way other devices are handled, I think we should
>> make it so you can use
>>
>> <controller type='scsi' model='virtio'/>
>>
>> as well, which of course would behave the same as the currently
>> available version. What do you think?
>>
>
> Yes I agree it would be nice to add that option. I suggest we make it a
> separate issue from this series though incase it's a contentious idea,
> v2 is already shaping up to be ~30 patches...
>
>>
>>> + case VIR_DOMAIN_DEVICE_CONTROLLER:
>>> + if (strstr(baseName, "scsi")) {
>>> + has_tmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL;
>>> + has_ntmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL;
>>> + tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL;
>>> + ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL;
>>> + break;
>>> + }
>>> + return 0;
>>
>> Using strstr() here is kinda crude, especially since the caller has
>> enough information to pass the appropriate virDomainControllerType
>> value, both in this case and later on for serial controllers.
>>
>> I'd say just add yet another argument to the function. Yeah, it
>> starts to get quite unsightly, but I'd really rather not resort to
>> string comparison when a nice, type safe enum will do.
>>
>
> Yeah it's hacky. Adding another arg here is going to add extra pain if
> when this is merged into qemuBuildVirtioStr, most callers are going have
> NULL arguments, and an increased chance of new future callers passing in
> incorrect values and accidentally triggering a wrong code path. I feel
> like this is another argument for the separated BuildTransitional or
> whatever, but I'm not sold either way so I'll stick with your suggestion
>
What do you think of this approach? See the attached two patches. It
adds a domain_conf.c function virDomainDeviceSetData which makes it
easier to create a virDomainDeviceDef. qemuBuildVirtioDevStr callers now
pass in a virDomainDeviceType and the specific DefPtr for their device
(virDomainDiskDefPtr, virDomainNetDefPtr etc). qemuBuildVirtioDevStr
then turns that into a virDomainDeviceDef and acts on that locally.
Saves having to extend the argument list several times (like this
example above, and the net->model string example). Seperately I think
virDomainDeviceSetData can be used to clean up some device interactions
elsewhere too
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2019-01-22 at 16:27 -0500, Cole Robinson wrote: > On 01/22/2019 12:39 PM, Cole Robinson wrote: > > On 01/21/2019 11:49 AM, Andrea Bolognani wrote: > > > Completely agree with the rationale here; however, in order to > > > really match the way other devices are handled, I think we should > > > make it so you can use > > > > > > <controller type='scsi' model='virtio'/> > > > > > > as well, which of course would behave the same as the currently > > > available version. What do you think? > > > > Yes I agree it would be nice to add that option. I suggest we make it a > > separate issue from this series though incase it's a contentious idea, > > v2 is already shaping up to be ~30 patches... I don't think that's going to be particularly controversial, and we should really make sure we get all the user-facing bits in at the same time IMHO, so I'd say go for it... If you're really unsure about it you can add that model in a separate patch right after this one, that way if someone happens not to like that we can drop it and otherwise we can squash them together before pushing. > > > Using strstr() here is kinda crude, especially since the caller has > > > enough information to pass the appropriate virDomainControllerType > > > value, both in this case and later on for serial controllers. > > > > > > I'd say just add yet another argument to the function. Yeah, it > > > starts to get quite unsightly, but I'd really rather not resort to > > > string comparison when a nice, type safe enum will do. > > > > Yeah it's hacky. Adding another arg here is going to add extra pain if > > when this is merged into qemuBuildVirtioStr, most callers are going have > > NULL arguments, and an increased chance of new future callers passing in > > incorrect values and accidentally triggering a wrong code path. I feel > > like this is another argument for the separated BuildTransitional or > > whatever, but I'm not sold either way so I'll stick with your suggestion > > What do you think of this approach? See the attached two patches. It > adds a domain_conf.c function virDomainDeviceSetData which makes it > easier to create a virDomainDeviceDef. qemuBuildVirtioDevStr callers now > pass in a virDomainDeviceType and the specific DefPtr for their device > (virDomainDiskDefPtr, virDomainNetDefPtr etc). qemuBuildVirtioDevStr > then turns that into a virDomainDeviceDef and acts on that locally. > > Saves having to extend the argument list several times (like this > example above, and the net->model string example). Seperately I think > virDomainDeviceSetData can be used to clean up some device interactions > elsewhere too I like it! I actually wanted to suggest something like that earlier, but for some reason I thought it would be more complicated than it turned out to be... Better yet, you don't even need to add that switch statement to qemuBuildVirtioDevStr(): you can just use virDomainDeviceGetInfo() instead, thus making the code shorter and nicer :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/23/2019 07:08 AM, Andrea Bolognani wrote: > On Tue, 2019-01-22 at 16:27 -0500, Cole Robinson wrote: >> On 01/22/2019 12:39 PM, Cole Robinson wrote: >>> On 01/21/2019 11:49 AM, Andrea Bolognani wrote: >>>> Completely agree with the rationale here; however, in order to >>>> really match the way other devices are handled, I think we should >>>> make it so you can use >>>> >>>> <controller type='scsi' model='virtio'/> >>>> >>>> as well, which of course would behave the same as the currently >>>> available version. What do you think? >>> >>> Yes I agree it would be nice to add that option. I suggest we make it a >>> separate issue from this series though incase it's a contentious idea, >>> v2 is already shaping up to be ~30 patches... > > I don't think that's going to be particularly controversial, and we > should really make sure we get all the user-facing bits in at the > same time IMHO, so I'd say go for it... If you're really unsure > about it you can add that model in a separate patch right after this > one, that way if someone happens not to like that we can drop it and > otherwise we can squash them together before pushing. > Alright will do >>>> Using strstr() here is kinda crude, especially since the caller has >>>> enough information to pass the appropriate virDomainControllerType >>>> value, both in this case and later on for serial controllers. >>>> >>>> I'd say just add yet another argument to the function. Yeah, it >>>> starts to get quite unsightly, but I'd really rather not resort to >>>> string comparison when a nice, type safe enum will do. >>> >>> Yeah it's hacky. Adding another arg here is going to add extra pain if >>> when this is merged into qemuBuildVirtioStr, most callers are going have >>> NULL arguments, and an increased chance of new future callers passing in >>> incorrect values and accidentally triggering a wrong code path. I feel >>> like this is another argument for the separated BuildTransitional or >>> whatever, but I'm not sold either way so I'll stick with your suggestion >> >> What do you think of this approach? See the attached two patches. It >> adds a domain_conf.c function virDomainDeviceSetData which makes it >> easier to create a virDomainDeviceDef. qemuBuildVirtioDevStr callers now >> pass in a virDomainDeviceType and the specific DefPtr for their device >> (virDomainDiskDefPtr, virDomainNetDefPtr etc). qemuBuildVirtioDevStr >> then turns that into a virDomainDeviceDef and acts on that locally. >> >> Saves having to extend the argument list several times (like this >> example above, and the net->model string example). Seperately I think >> virDomainDeviceSetData can be used to clean up some device interactions >> elsewhere too > > I like it! I actually wanted to suggest something like that earlier, > but for some reason I thought it would be more complicated than it > turned out to be... > > Better yet, you don't even need to add that switch statement to > qemuBuildVirtioDevStr(): you can just use virDomainDeviceGetInfo() > instead, thus making the code shorter and nicer :) > Ah good catch, though the switch statement will end up in the final result anyways with all the transitional additions Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2019-01-23 at 07:52 -0500, Cole Robinson wrote: > On 01/23/2019 07:08 AM, Andrea Bolognani wrote: > > Better yet, you don't even need to add that switch statement to > > qemuBuildVirtioDevStr(): you can just use virDomainDeviceGetInfo() > > instead, thus making the code shorter and nicer :) > > Ah good catch, though the switch statement will end up in the final > result anyways with all the transitional additions That's a very solid point :) Sorry for the noise. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/23/2019 07:52 AM, Cole Robinson wrote: > On 01/23/2019 07:08 AM, Andrea Bolognani wrote: >> On Tue, 2019-01-22 at 16:27 -0500, Cole Robinson wrote: >>> On 01/22/2019 12:39 PM, Cole Robinson wrote: >>>> On 01/21/2019 11:49 AM, Andrea Bolognani wrote: >>>>> Completely agree with the rationale here; however, in order to >>>>> really match the way other devices are handled, I think we should >>>>> make it so you can use >>>>> >>>>> <controller type='scsi' model='virtio'/> >>>>> >>>>> as well, which of course would behave the same as the currently >>>>> available version. What do you think? >>>> >>>> Yes I agree it would be nice to add that option. I suggest we make it a >>>> separate issue from this series though incase it's a contentious idea, >>>> v2 is already shaping up to be ~30 patches... >> >> I don't think that's going to be particularly controversial, and we >> should really make sure we get all the user-facing bits in at the >> same time IMHO, so I'd say go for it... If you're really unsure >> about it you can add that model in a separate patch right after this >> one, that way if someone happens not to like that we can drop it and >> otherwise we can squash them together before pushing. >> > > Alright will do > I'm realizing now I forgot this, sorry. I'll add it to my todo list Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.