On 8/30/19 5:40 PM, jcfaracco@xxxxxxxxx wrote: > From: Julio Faracco <jcfaracco@xxxxxxxxx> > > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
