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

Fri May 25 12:10:29 UTC 2018

On 05/25/2018 02:04 PM, John Ferlan wrote:
> 
> 
> 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