On 05/25/2018 07:39 AM, Michal Privoznik wrote:
On 05/25/2018 01:10 PM, John Ferlan wrote:
On 05/25/2018 04:24 AM, Michal Privoznik wrote:
On 05/17/2018 02:42 PM, John Ferlan wrote:
Second reposting of:
https://www.redhat.com/archives/libvir-list/2018-May/msg00813.html
To update patches with more conflicts for patch 2 (capabilities) and patch 6 (news)
Cover from the v3 posting:
v2: https://www.redhat.com/archives/libvir-list/2018-April/msg02234.html
Changes since v2:
* Essentially handle comments from code review of original series from comments received for patch 6:
https://www.redhat.com/archives/libvir-list/2018-April/msg02240.html
It's a somewhat simplified approach removing the ABI checks and the adjustment to the genid value as part of domain def copy.
* (NEW) Patch 5 - add a 'genid' domain capability (similar to how Cole added support for vmcoreinfo). Since the apps need a way to determine whether this is enabled, this seems to be the best way.
John Ferlan (6): conf: Add VM Generation ID parse/format support qemu: Add VM Generation ID device capability qemu: Alter VM Generation ID for specific startup/launch transitions qemu: Add VM Generation ID to qemu command line domcaps: Add 'genid' to domain capabilities docs: Add news article for VM Generation ID
docs/formatdomain.html.in | 27 +++++++++++ docs/formatdomaincaps.html.in | 7 ++- docs/news.xml | 13 ++++++ docs/schemas/domaincaps.rng | 7 +++ docs/schemas/domaincommon.rng | 8 ++++ src/conf/domain_capabilities.c | 3 ++ src/conf/domain_capabilities.h | 1 + src/conf/domain_conf.c | 54 ++++++++++++++++++++++ src/conf/domain_conf.h | 5 ++ src/qemu/qemu_capabilities.c | 4 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 24 ++++++++++ src/qemu/qemu_driver.c | 17 +++++-- src/qemu/qemu_process.c | 46 +++++++++++++++++- src/qemu/qemu_process.h | 1 + tests/domaincapsschemadata/basic.xml | 1 + tests/domaincapsschemadata/bhyve_basic.x86_64.xml | 1 + tests/domaincapsschemadata/bhyve_fbuf.x86_64.xml | 1 + tests/domaincapsschemadata/bhyve_uefi.x86_64.xml | 1 + tests/domaincapsschemadata/full.xml | 1 + tests/domaincapsschemadata/libxl-xenfv-usb.xml | 1 + tests/domaincapsschemadata/libxl-xenfv.xml | 1 + tests/domaincapsschemadata/libxl-xenpv-usb.xml | 1 + tests/domaincapsschemadata/libxl-xenpv.xml | 1 + tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml | 1 + .../qemu_2.12.0-virt.aarch64.xml | 1 + tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml | 1 + tests/domaincapsschemadata/qemu_2.12.0.s390x.xml | 1 + tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml | 1 + .../qemu_2.6.0-virt.aarch64.xml | 1 + tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml | 1 + tests/domaincapsschemadata/qemu_2.6.0.ppc64.xml | 1 + tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.7.0.s390x.xml | 1 + .../domaincapsschemadata/qemu_2.8.0-tcg.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.8.0.s390x.xml | 1 + tests/domaincapsschemadata/qemu_2.8.0.x86_64.xml | 1 + .../domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml | 1 + .../domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + .../qemuxml2argvdata/genid-auto.x86_64-latest.args | 30 ++++++++++++ tests/qemuxml2argvdata/genid-auto.xml | 32 +++++++++++++ tests/qemuxml2argvdata/genid.x86_64-latest.args | 30 ++++++++++++ tests/qemuxml2argvdata/genid.xml | 32 +++++++++++++ tests/qemuxml2argvtest.c | 4 ++ tests/qemuxml2xmloutdata/genid-active.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-auto-active.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-auto-inactive.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-inactive.xml | 32 +++++++++++++ tests/qemuxml2xmltest.c | 5 +- 53 files changed, 500 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2argvdata/genid-auto.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/genid-auto.xml create mode 100644 tests/qemuxml2argvdata/genid.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/genid.xml create mode 100644 tests/qemuxml2xmloutdata/genid-active.xml create mode 100644 tests/qemuxml2xmloutdata/genid-auto-active.xml create mode 100644 tests/qemuxml2xmloutdata/genid-auto-inactive.xml create mode 100644 tests/qemuxml2xmloutdata/genid-inactive.xml
I like the patches, I do. I'd ACK them but some discussion is needed first in my opinion.
Discussion about what? Honestly this "version" of the patches has been languishing for the entire month without any review. I know "everyone" is "busy" with their own stuff and facing their own deadlines so I don't expect immediate review, but a whole month without any feedback is a bitter pill to swallow. Now that we reach the end of the month and release time - the design is being called into question. That's fine - although IMO I got review on the original design from the RFC posted in March and the v1 posted in early April. The v2 series posted a couple weeks later gave a lot more important feedback and resulted in the adjustments posted here. So as noted in another response, the v2 review gave the best feedback and is the basis for this simplified approach.
I meant answers to my questions. Esp. on the capability check and usage of flags. If you fix the capability check like I'm suggesting in 4/6 you have my ACK for the whole patch set.
Ironically, as part of review of v2 patch 10, I was asked to remove the check from qemuBuildVMGenIDCommandLine: https://www.redhat.com/archives/libvir-list/2018-April/msg02253.html Perhaps it's best to just move it from qemu_process to qemu_command - that just means passing @qemuCaps into qemuBuildVMGenIDCommandLine and using it as well as removing @priv from qemuProcessGenID. It really doesn't matter in qemu_process whether the capability exists, it was an optimization to put it there rather than an error path in qemu_command... and with that change GenIDCommandLine now could return -1 or 0; whereas, without the caps check it was only ever returning 0. I can repost the entire series if so desired. Tks - John
Also, sorry for leaving these patches so long without any review.
Michal