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.
I don't think it's needed. Just fix and push.
Michal