Re: [libvirt] [REPOSTv2 PATCH v3 0/6] Add support for VM Generation ID (vmgenid)

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.

Also, sorry for leaving these patches so long without any review. Michal