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

jcfaracco@gmail.com posted 10 patches 4 years, 7 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190830214101.8759-1-jcfaracco@gmail.com
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
[libvirt] [PATCH v2 00/10] Adding resolution properties for video models
Posted by jcfaracco@gmail.com 4 years, 7 months ago
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

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

-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 00/10] Adding resolution properties for video models
Posted by Cole Robinson 4 years, 6 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list