[libvirt PATCH v3 0/2] Refuse PCI Address for ramfb device definition
Changes in this version: - Add the test case input file - modify the test itself to properly fail when an input file is missing. Jonathon Jongsma (2): qemu: ramfb video device doesn't support PCI address tests: ensure failure if input file doesn't exist src/qemu/qemu_validate.c | 8 +++++ ...video-ramfb-display-device-pci-address.xml | 30 +++++++++++++++++++ tests/qemuxml2argvtest.c | 7 +++++ 3 files changed, 45 insertions(+) create mode 100644 tests/qemuxml2argvdata/video-ramfb-display-device-pci-address.xml -- 2.21.3
Although a ramfb video device is not a PCI device, we don't currently report an error for ramfb device definitions containing a PCI address. However, a guest configured with such a device will fail to start: # virsh start test1 error: Failed to start domain test1 error: internal error: qemu unexpectedly closed the monitor: 2020-06-16T05:23:02.759221Z qemu-kvm: -device ramfb,id=video0,bus=pcie.0,addr=0x1: Device 'ramfb' can't go on PCIE bus A better approach is to reject any device definitions that contain PCI addresses. While this is a change in behavior, any existing configurations were non-functional. https://bugzilla.redhat.com/show_bug.cgi?id=1847259 Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_validate.c | 8 +++++ ...video-ramfb-display-device-pci-address.xml | 30 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 39 insertions(+) create mode 100644 tests/qemuxml2argvdata/video-ramfb-display-device-pci-address.xml diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 5082a29dc7..b13c03759e 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1925,6 +1925,14 @@ qemuValidateDomainDeviceDefVideo(const virDomainVideoDef *video, if (qemuValidateDomainVirtioOptions(video->virtio, qemuCaps) < 0) return -1; + if (video->type == VIR_DOMAIN_VIDEO_TYPE_RAMFB && + video->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'address' is not supported for 'ramfb' video devices")); + return -1; + } + + return 0; } diff --git a/tests/qemuxml2argvdata/video-ramfb-display-device-pci-address.xml b/tests/qemuxml2argvdata/video-ramfb-display-device-pci-address.xml new file mode 100644 index 0000000000..db110560ac --- /dev/null +++ b/tests/qemuxml2argvdata/video-ramfb-display-device-pci-address.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i386</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source file='/var/lib/libvirt/images/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <video> + <model type='ramfb' /> + <address type='pci' domain='0x000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1195f9c982..f2522fa530 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2276,6 +2276,7 @@ mymain(void) QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS); DO_TEST_CAPS_LATEST("video-bochs-display-device"); DO_TEST_CAPS_LATEST("video-ramfb-display-device"); + DO_TEST_CAPS_LATEST_PARSE_ERROR("video-ramfb-display-device-pci-address"); DO_TEST("video-none-device", QEMU_CAPS_VNC); DO_TEST_PARSE_ERROR("video-invalid-multiple-devices", NONE); -- 2.21.3
When using the DO_TEST_PARSE_ERROR() macro, a failure to parse the input file is considered a successful test. However, if the input file is totally missing, that should be distinguished from a parsing error and not be treated as a test success. The function virDomainDefParseFile() simply returns NULL for any parse failure, including a missing file. So we need to explicitly check whether the file exists first, and fail the test if it is missing. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- tests/qemuxml2argvtest.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index f2522fa530..248ce07811 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -608,6 +608,12 @@ testCompareXMLToArgv(const void *data) if (!(vm = virDomainObjNew(driver.xmlopt))) goto cleanup; + if (!virFileExists(info->infile)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Input file '%s' not found", info->infile); + goto cleanup; + } + parseFlags |= VIR_DOMAIN_DEF_PARSE_INACTIVE; if (!(vm->def = virDomainDefParseFile(info->infile, driver.xmlopt, -- 2.21.3
On 6/25/20 4:18 PM, Jonathon Jongsma wrote:
Changes in this version: - Add the test case input file - modify the test itself to properly fail when an input file is missing.
Jonathon Jongsma (2): qemu: ramfb video device doesn't support PCI address tests: ensure failure if input file doesn't exist
src/qemu/qemu_validate.c | 8 +++++ ...video-ramfb-display-device-pci-address.xml | 30 +++++++++++++++++++ tests/qemuxml2argvtest.c | 7 +++++ 3 files changed, 45 insertions(+) create mode 100644 tests/qemuxml2argvdata/video-ramfb-display-device-pci-address.xml
For both: Reviewed-by: Laine Stump <laine@redhat.com> and pushed (...aaaand I just realized I forgot to add the Reviewed-by: tag before pushing :-/. Ah well, it shows me as the committer, so you won't have to *totally* take the fall if something blows up :-))
participants (2)
-
Jonathon Jongsma -
Laine Stump