4:54 p.m.
On 8/30/19 5:40 PM, jcfaracco(a)gmail.com wrote:
From: Julio Faracco <jcfaracco(a)gmail.com>
This serie adds 'xres' and 'yres' QEMU display properties into a new
element called 'resolution'. This element is covered by model element:
<model type='foo'>
<resolution x='800' y='600'/>
</model>
Only types VGA, QXL, Virtio and Bochs support passing resolution to
driver. That's why some test cases were added too.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1485793
Hi Julio, thanks for working on this! The XML looks fine to me
As a general suggestion, if you send a series and forgot to add XML
files like for this one, better IMO to send a v2 straight away. Makes it
easier for reviewers to pull the patches down and test them directly
For formatting the series I think this patch is a good guide, make
domain_conf.c changes self contained with docs/ and a test/ addition
https://www.redhat.com/archives/libvir-list/2019-May/msg00820.html
I don't think hardcoding the video model support checks is really worth
it in this case. It complicates the code and testing, only for the gain
of catching an error at XML define time vs startup time. IMO for this
case it is fine to just unconditionally pass xres/yres to the -device
string, and let qemu fail if it's not supported. This simplifies the
qemu_command.c code, and then you can use a single test case to get code
coverage there. All the qemu_capabilities changes can then be dropped.
Others may have different opinions though
I'll add some specific inline comments too
Thanks,
Cole
Julio Faracco (10):
docs: Adding 'xres' and 'yres' into qxl XML definition
qemu: Include {xres,yres} QEMU capabilities for video models
conf: Adding resolution property for model element
qemu: Include {xres,yres} for QEMU command line
tests: Include {xres,yres} QEMU capabilities into tests
tests: Include bochs-display as capability test too
tests: Introduce resolution test for QXL model
tests: Introduce resolution test for Virtio model
tests: Introduce resolution test for Bochs model
tests: Introduce resolution test for VGA model
docs/schemas/domaincommon.rng | 10 ++
src/conf/domain_conf.c | 75 +++++++-
src/conf/domain_conf.h | 5 +
src/conf/virconftypes.h | 3 +
src/qemu/qemu_capabilities.c | 16 ++
src/qemu/qemu_capabilities.h | 2 +
src/qemu/qemu_command.c | 20 +++
.../caps_2.10.0.aarch64.xml | 2 +
.../caps_2.10.0.ppc64.xml | 2 +
.../caps_2.10.0.s390x.xml | 2 +
.../caps_2.10.0.x86_64.xml | 2 +
.../caps_2.11.0.s390x.xml | 2 +
.../caps_2.11.0.x86_64.xml | 2 +
.../caps_2.12.0.aarch64.xml | 2 +
.../caps_2.12.0.ppc64.xml | 2 +
.../caps_2.12.0.s390x.xml | 2 +
.../caps_2.12.0.x86_64.xml | 2 +
.../caps_3.0.0.ppc64.replies | 139 +++++++++++----
.../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 2 +
.../caps_3.0.0.riscv32.xml | 2 +
.../caps_3.0.0.riscv64.xml | 2 +
.../qemucapabilitiesdata/caps_3.0.0.s390x.xml | 2 +
.../caps_3.0.0.x86_64.replies | 167 +++++++++++++-----
.../caps_3.0.0.x86_64.xml | 2 +
.../caps_3.1.0.ppc64.replies | 139 +++++++++++----
.../qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 2 +
.../caps_3.1.0.x86_64.replies | 167 +++++++++++++-----
.../caps_3.1.0.x86_64.xml | 2 +
.../caps_4.0.0.aarch64.replies | 139 +++++++++++----
.../caps_4.0.0.aarch64.xml | 2 +
.../caps_4.0.0.ppc64.replies | 139 +++++++++++----
.../qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 2 +
.../caps_4.0.0.riscv32.replies | 131 +++++++++++---
.../caps_4.0.0.riscv32.xml | 2 +
.../caps_4.0.0.riscv64.replies | 131 +++++++++++---
.../caps_4.0.0.riscv64.xml | 2 +
.../qemucapabilitiesdata/caps_4.0.0.s390x.xml | 2 +
.../caps_4.0.0.x86_64.replies | 167 +++++++++++++-----
.../caps_4.0.0.x86_64.xml | 2 +
.../caps_4.1.0.x86_64.replies | 159 ++++++++++++-----
.../caps_4.1.0.x86_64.xml | 2 +
...ochs-display-resolution.x86_64-latest.args | 35 ++++
.../video-bochs-display-resolution.xml | 33 ++++
.../video-qxl-resolution.x86_64-latest.args | 35 ++++
.../qemuxml2argvdata/video-qxl-resolution.xml | 33 ++++
...o-virtio-gpu-resolution.x86_64-latest.args | 35 ++++
.../video-virtio-gpu-resolution.xml | 33 ++++
tests/qemuxml2argvtest.c | 4 +
...bochs-display-resolution.x86_64-latest.xml | 33 ++++
.../video-qxl-resolution.x86_64-latest.xml | 33 ++++
...eo-virtio-gpu-resolution.x86_64-latest.xml | 33 ++++
tests/qemuxml2xmltest.c | 5 +
52 files changed, 1604 insertions(+), 365 deletions(-)
create mode 100644
tests/qemuxml2argvdata/video-bochs-display-resolution.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/video-bochs-display-resolution.xml
create mode 100644 tests/qemuxml2argvdata/video-qxl-resolution.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/video-qxl-resolution.xml
create mode 100644
tests/qemuxml2argvdata/video-virtio-gpu-resolution.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-resolution.xml
create mode 100644
tests/qemuxml2xmloutdata/video-bochs-display-resolution.x86_64-latest.xml
create mode 100644 tests/qemuxml2xmloutdata/video-qxl-resolution.x86_64-latest.xml
create mode 100644
tests/qemuxml2xmloutdata/video-virtio-gpu-resolution.x86_64-latest.xml
- Cole