On 8/30/19 5:40 PM, jcfaracco@gmail.com wrote:
From: Julio Faracco <jcfaracco@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