[libvirt] [PATCH v2 00/10] Adding resolution properties for video models

[Cole Robinson]

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