[libvirt] [PATCH 0/3] conf: <vmcoreinfo> tweaks
Patch #1 changes the vmcoreinfo XML schema slightly to be more future proof, this is technically an API break but I'm not sure it matters, see the patch for more details. Patch #3 adds vmcoreinfo reporting to domain capabilities. The schema doesn't follow the typical pattern so if anyone has other ideas please comment Cole Robinson (3): conf: format/parse <vmcoreinfo> as tristate conf: Add a comment warning about boolean feature XML domain_capabilities: Report <vmcoreinfo> support docs/formatdomaincaps.html.in | 5 +++++ docs/schemas/domaincaps.rng | 7 +++++++ docs/schemas/domaincommon.rng | 6 +++++- src/conf/domain_capabilities.c | 2 ++ src/conf/domain_capabilities.h | 1 + src/conf/domain_conf.c | 6 ++++-- src/qemu/qemu_capabilities.c | 3 +++ 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 + tests/domaincapsschemadata/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 + tests/domaincapsschemadata/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 + tests/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 + tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml | 1 + tests/qemuxml2xmloutdata/vmcoreinfo.xml | 2 +- 33 files changed, 53 insertions(+), 4 deletions(-) -- 2.17.0
<features><vmcoreinfo/> is a bare boolean XML property. We don't really use this format anymore and instead prefer tristate <X state=on|off/> since it's required for modeling on/off/default. If for example future qemu started enabling vmcoreinfo by default we wouldn't have any way for the user to turn this off. Convert it to tristate. For writing XML this is semanticly the same, <vmcoreinfo/> is processed as <vmcoreinfo state='on'/>. For apps reading guest XML this is technically an API change, as they might misinterpret <vmcoreinfo state='off'/>, however this has only been present in libvirt since 3.10.0 and I don't think any apps are dependent on this yet Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 4 ++-- tests/qemuxml2xmloutdata/vmcoreinfo.xml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4dad8e3b2..648057ad4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19281,7 +19281,6 @@ virDomainDefParseXML(xmlDocPtr xml, case VIR_DOMAIN_FEATURE_VIRIDIAN: case VIR_DOMAIN_FEATURE_PRIVNET: case VIR_DOMAIN_FEATURE_HYPERV: - case VIR_DOMAIN_FEATURE_VMCOREINFO: case VIR_DOMAIN_FEATURE_KVM: def->features[val] = VIR_TRISTATE_SWITCH_ON; break; @@ -19300,6 +19299,7 @@ virDomainDefParseXML(xmlDocPtr xml, } break; + case VIR_DOMAIN_FEATURE_VMCOREINFO: case VIR_DOMAIN_FEATURE_HAP: case VIR_DOMAIN_FEATURE_PMU: case VIR_DOMAIN_FEATURE_PVSPINLOCK: @@ -26870,7 +26870,6 @@ virDomainDefFormatInternal(virDomainDefPtr def, case VIR_DOMAIN_FEATURE_ACPI: case VIR_DOMAIN_FEATURE_PAE: case VIR_DOMAIN_FEATURE_VIRIDIAN: - case VIR_DOMAIN_FEATURE_VMCOREINFO: case VIR_DOMAIN_FEATURE_PRIVNET: switch ((virTristateSwitch) def->features[i]) { case VIR_TRISTATE_SWITCH_ABSENT: @@ -26891,6 +26890,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, break; + case VIR_DOMAIN_FEATURE_VMCOREINFO: case VIR_DOMAIN_FEATURE_HAP: case VIR_DOMAIN_FEATURE_PMU: case VIR_DOMAIN_FEATURE_PVSPINLOCK: diff --git a/tests/qemuxml2xmloutdata/vmcoreinfo.xml b/tests/qemuxml2xmloutdata/vmcoreinfo.xml index d0cd2f2ce..48b75d7d4 100644 --- a/tests/qemuxml2xmloutdata/vmcoreinfo.xml +++ b/tests/qemuxml2xmloutdata/vmcoreinfo.xml @@ -9,7 +9,7 @@ <boot dev='hd'/> </os> <features> - <vmcoreinfo/> + <vmcoreinfo state='on'/> </features> <clock offset='utc'/> <on_poweroff>destroy</on_poweroff> -- 2.17.0
On 04/17/2018 02:40 PM, Cole Robinson wrote:
<features><vmcoreinfo/> is a bare boolean XML property. We don't really use this format anymore and instead prefer tristate <X state=on|off/> since it's required for modeling on/off/default. If for example future qemu started enabling vmcoreinfo by default we wouldn't have any way for the user to turn this off.
Convert it to tristate. For writing XML this is semanticly the same, <vmcoreinfo/> is processed as <vmcoreinfo state='on'/>.
For apps reading guest XML this is technically an API change, as they might misinterpret <vmcoreinfo state='off'/>, however this has only been present in libvirt since 3.10.0 and I don't think any apps are dependent on this yet
^^ perhaps partially a --- type comment - how much you leave in the commit is your call
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 4 ++-- tests/qemuxml2xmloutdata/vmcoreinfo.xml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
FWIW: Looks like HAP changed too (commit id '9d243e08') - so from the aspect of precedence, this would seem to be OK then. Two things missing from this commit then would be the change to formatdomain.html.in to describe the XML format and domaincommon.rng (e.g. from patch 2). I'd be fine if patch 1 and 2 were merged since all that would be left in 2 would be the comment addition which would seem to be a good idea for this patch. Please wait for 4.4.0 before pushing and with the formatdomain change consider this, Reviewed-by: John Ferlan <jferlan@redhat.com> John
This is the old style and we really shouldn't be adding any more examples like this. Add a comment to warn devs away Signed-off-by: Cole Robinson <crobinso@redhat.com> --- docs/schemas/domaincommon.rng | 6 +++++- src/conf/domain_conf.c | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4cab55f05..015c5b737 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -5036,7 +5036,11 @@ <define name="vmcoreinfo"> <element name="vmcoreinfo"> - <empty/> + <optional> + <attribute name="state"> + <ref name="virOnOff"/> + </attribute> + </optional> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 648057ad4..e303c503e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -26871,6 +26871,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, case VIR_DOMAIN_FEATURE_PAE: case VIR_DOMAIN_FEATURE_VIRIDIAN: case VIR_DOMAIN_FEATURE_PRIVNET: + /* NOTE: This is for old style <opt/> booleans. New XML + * should use the explicit state=on|off output below */ switch ((virTristateSwitch) def->features[i]) { case VIR_TRISTATE_SWITCH_ABSENT: break; -- 2.17.0
On 04/17/2018 02:40 PM, Cole Robinson wrote:
This is the old style and we really shouldn't be adding any more examples like this. Add a comment to warn devs away
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- docs/schemas/domaincommon.rng | 6 +++++- src/conf/domain_conf.c | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-)
The domaincommon.rng change should go in patch 1. I'll leave it up to you whether to combine the domain_conf.c change as well. Reviewed-by: John Ferlan <jferlan@redhat.com> John
Report domaincaps <features><vmcoreinfo supported='yes'/> if the guest config accepts <features><vmcoreinfo state='on'/> Signed-off-by: Cole Robinson <crobinso@redhat.com> --- This bucks the domaincapabilities trend of always having a child enum if supported='yes'. Following that trend we would give us this XML when vmcoreinfo is supported: <vmcoreinfo supported='yes'> <enum name='state'> <value>on</value> <value>off</value> </enum> </vmcoreinfo> Which is verbose but fine. But it's unclear what we do in the case when vmcoreinfo isn't supported... do we do: <vmcoreinfo supported='no'/> which may not be entirely accurate because the code will still accept <vmcoreinfo enabled='off'/>. Or do we do: <vmcoreinfo supported='yes'> <enum name='state'> <value>off</value> </enum> </vmcoreinfo> Which is weird IMO. I'm not certain what the semantics of 'supported' are meant to be so I went with this minimal option but I'm not tied to it docs/formatdomaincaps.html.in | 5 +++++ docs/schemas/domaincaps.rng | 7 +++++++ src/conf/domain_capabilities.c | 2 ++ src/conf/domain_capabilities.h | 1 + src/qemu/qemu_capabilities.c | 3 +++ 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 + tests/domaincapsschemadata/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 + tests/domaincapsschemadata/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 + tests/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 + tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml | 1 + 30 files changed, 43 insertions(+) diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in index 6bfcaf61c..acdc1cf8a 100644 --- a/docs/formatdomaincaps.html.in +++ b/docs/formatdomaincaps.html.in @@ -417,6 +417,7 @@ <value>3</value> </enum> </gic> + <vmcoreinfo supported='yes'/> </features> </domainCapabilities> </pre> @@ -441,5 +442,9 @@ <code>gic</code> element.</dd> </dl> + <h4><a id="elementsvmcoreinfo">vmcoreinfo</a></h4> + + <p>Reports whether the vmcoreinfo feature can be enabled</p> + </body> </html> diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng index 39053181e..bace0e44a 100644 --- a/docs/schemas/domaincaps.rng +++ b/docs/schemas/domaincaps.rng @@ -173,6 +173,7 @@ <element name='features'> <interleave> <ref name='gic'/> + <ref name='vmcoreinfo'/> </interleave> </element> </define> @@ -184,6 +185,12 @@ </element> </define> + <define name='vmcoreinfo'> + <element name='vmcoreinfo'> + <ref name='supported'/> + </element> + </define> + <define name='value'> <zeroOrMore> <element name='value'> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index f7d9be50f..ccdccd695 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -587,6 +587,8 @@ virDomainCapsFormat(virDomainCapsPtr const caps) virBufferAdjustIndent(&buf, 2); virDomainCapsFeatureGICFormat(&buf, &caps->gic); + virBufferAsprintf(&buf, "<vmcoreinfo supported='%s'/>\n", + caps->vmcoreinfo ? "yes" : "no"); virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</features>\n"); diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index fa4c1e442..5bb028f63 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -157,6 +157,7 @@ struct _virDomainCaps { /* add new domain devices here */ virDomainCapsFeatureGIC gic; + bool vmcoreinfo; /* add new domain features here */ }; diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index f379fc6d2..0543c0194 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4830,6 +4830,9 @@ virQEMUCapsFillDomainCaps(virCapsPtr caps, domCaps->maxvcpus = MIN(domCaps->maxvcpus, hostmaxvcpus); } + domCaps->vmcoreinfo = virQEMUCapsGet(qemuCaps, + QEMU_CAPS_DEVICE_VMCOREINFO); + if (virQEMUCapsFillDomainOSCaps(os, firmwares, nfirmwares) < 0 || virQEMUCapsFillDomainCPUCaps(caps, qemuCaps, domCaps) < 0 || virQEMUCapsFillDomainDeviceDiskCaps(qemuCaps, diff --git a/tests/domaincapsschemadata/basic.xml b/tests/domaincapsschemadata/basic.xml index 6b788d914..e937b3607 100644 --- a/tests/domaincapsschemadata/basic.xml +++ b/tests/domaincapsschemadata/basic.xml @@ -17,5 +17,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/bhyve_basic.x86_64.xml b/tests/domaincapsschemadata/bhyve_basic.x86_64.xml index 95a8038c2..70e36e97e 100644 --- a/tests/domaincapsschemadata/bhyve_basic.x86_64.xml +++ b/tests/domaincapsschemadata/bhyve_basic.x86_64.xml @@ -27,5 +27,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/bhyve_fbuf.x86_64.xml b/tests/domaincapsschemadata/bhyve_fbuf.x86_64.xml index 662d0db9a..19b76da45 100644 --- a/tests/domaincapsschemadata/bhyve_fbuf.x86_64.xml +++ b/tests/domaincapsschemadata/bhyve_fbuf.x86_64.xml @@ -44,5 +44,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/bhyve_uefi.x86_64.xml b/tests/domaincapsschemadata/bhyve_uefi.x86_64.xml index f8c79b99b..c278df038 100644 --- a/tests/domaincapsschemadata/bhyve_uefi.x86_64.xml +++ b/tests/domaincapsschemadata/bhyve_uefi.x86_64.xml @@ -36,5 +36,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/full.xml b/tests/domaincapsschemadata/full.xml index ab6ef9f2e..93bdad2eb 100644 --- a/tests/domaincapsschemadata/full.xml +++ b/tests/domaincapsschemadata/full.xml @@ -107,5 +107,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/libxl-xenfv-usb.xml b/tests/domaincapsschemadata/libxl-xenfv-usb.xml index 6a9e3d96f..dfa992cb7 100644 --- a/tests/domaincapsschemadata/libxl-xenfv-usb.xml +++ b/tests/domaincapsschemadata/libxl-xenfv-usb.xml @@ -70,5 +70,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/libxl-xenfv.xml b/tests/domaincapsschemadata/libxl-xenfv.xml index d48e69948..c74632a13 100644 --- a/tests/domaincapsschemadata/libxl-xenfv.xml +++ b/tests/domaincapsschemadata/libxl-xenfv.xml @@ -69,5 +69,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/libxl-xenpv-usb.xml b/tests/domaincapsschemadata/libxl-xenpv-usb.xml index d1a3918ef..8277a9efd 100644 --- a/tests/domaincapsschemadata/libxl-xenpv-usb.xml +++ b/tests/domaincapsschemadata/libxl-xenpv-usb.xml @@ -60,5 +60,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/libxl-xenpv.xml b/tests/domaincapsschemadata/libxl-xenpv.xml index c0e3193fe..c816b2053 100644 --- a/tests/domaincapsschemadata/libxl-xenpv.xml +++ b/tests/domaincapsschemadata/libxl-xenpv.xml @@ -59,5 +59,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml b/tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml index 8d1ad8657..d79e8015f 100644 --- a/tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml +++ b/tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml @@ -108,5 +108,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/qemu_2.12.0-virt.aarch64.xml b/tests/domaincapsschemadata/qemu_2.12.0-virt.aarch64.xml index 9cba942fb..982f31a9a 100644 --- a/tests/domaincapsschemadata/qemu_2.12.0-virt.aarch64.xml +++ b/tests/domaincapsschemadata/qemu_2.12.0-virt.aarch64.xml @@ -115,5 +115,6 @@ <value>3</value> </enum> </gic> + <vmcoreinfo supported='yes'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml b/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml index 4029e9e38..0aa14b127 100644 --- a/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml +++ b/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml @@ -77,5 +77,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/qemu_2.12.0.s390x.xml b/tests/domaincapsschemadata/qemu_2.12.0.s390x.xml index 50680c1ef..693b6dc7a 100644 --- a/tests/domaincapsschemadata/qemu_2.12.0.s390x.xml +++ b/tests/domaincapsschemadata/qemu_2.12.0.s390x.xml @@ -74,5 +74,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml b/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml index 851d914bc..797ecfae7 100644 --- a/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml +++ b/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml @@ -139,5 +139,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='yes'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/qemu_2.6.0-virt.aarch64.xml b/tests/domaincapsschemadata/qemu_2.6.0-virt.aarch64.xml index 7c019b230..5e4ac26f7 100644 --- a/tests/domaincapsschemadata/qemu_2.6.0-virt.aarch64.xml +++ b/tests/domaincapsschemadata/qemu_2.6.0-virt.aarch64.xml @@ -112,5 +112,6 @@ <value>2</value> </enum> </gic> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml b/tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml index 3b1428062..5bc69e717 100644 --- a/tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml +++ b/tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml @@ -108,5 +108,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/qemu_2.6.0.ppc64.xml b/tests/domaincapsschemadata/qemu_2.6.0.ppc64.xml index 98c72d36d..0543e3f56 100644 --- a/tests/domaincapsschemadata/qemu_2.6.0.ppc64.xml +++ b/tests/domaincapsschemadata/qemu_2.6.0.ppc64.xml @@ -81,5 +81,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml b/tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml index de8188623..21198b500 100644 --- a/tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml +++ b/tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml @@ -113,5 +113,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/qemu_2.7.0.s390x.xml b/tests/domaincapsschemadata/qemu_2.7.0.s390x.xml index b93d00ece..1e6140091 100644 --- a/tests/domaincapsschemadata/qemu_2.7.0.s390x.xml +++ b/tests/domaincapsschemadata/qemu_2.7.0.s390x.xml @@ -74,5 +74,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/qemu_2.8.0-tcg.x86_64.xml b/tests/domaincapsschemadata/qemu_2.8.0-tcg.x86_64.xml index d860cd833..8ae794f69 100644 --- a/tests/domaincapsschemadata/qemu_2.8.0-tcg.x86_64.xml +++ b/tests/domaincapsschemadata/qemu_2.8.0-tcg.x86_64.xml @@ -114,5 +114,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/qemu_2.8.0.s390x.xml b/tests/domaincapsschemadata/qemu_2.8.0.s390x.xml index ee40d1e84..ac6317755 100644 --- a/tests/domaincapsschemadata/qemu_2.8.0.s390x.xml +++ b/tests/domaincapsschemadata/qemu_2.8.0.s390x.xml @@ -155,5 +155,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/qemu_2.8.0.x86_64.xml b/tests/domaincapsschemadata/qemu_2.8.0.x86_64.xml index 33161f7b8..d227926c6 100644 --- a/tests/domaincapsschemadata/qemu_2.8.0.x86_64.xml +++ b/tests/domaincapsschemadata/qemu_2.8.0.x86_64.xml @@ -114,5 +114,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml b/tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml index 07e1db641..513b71c1b 100644 --- a/tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml +++ b/tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml @@ -122,5 +122,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml b/tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml index 6c2bbac19..38fddf586 100644 --- a/tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml +++ b/tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml @@ -146,5 +146,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml b/tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml index 2fc3b7213..501fcc682 100644 --- a/tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml +++ b/tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml @@ -123,5 +123,6 @@ </devices> <features> <gic supported='no'/> + <vmcoreinfo supported='no'/> </features> </domainCapabilities> -- 2.17.0
On 04/17/2018 02:40 PM, Cole Robinson wrote:
Report domaincaps <features><vmcoreinfo supported='yes'/> if the guest config accepts <features><vmcoreinfo state='on'/>
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- This bucks the domaincapabilities trend of always having a child enum if supported='yes'. Following that trend we would give us this XML when vmcoreinfo is supported:
<vmcoreinfo supported='yes'> <enum name='state'> <value>on</value> <value>off</value> </enum> </vmcoreinfo>
Which is verbose but fine. But it's unclear what we do in the case when vmcoreinfo isn't supported... do we do:
<vmcoreinfo supported='no'/>
which may not be entirely accurate because the code will still accept <vmcoreinfo enabled='off'/>. Or do we do:
<vmcoreinfo supported='yes'> <enum name='state'> <value>off</value> </enum> </vmcoreinfo>
Which is weird IMO. I'm not certain what the semantics of 'supported' are meant to be so I went with this minimal option but I'm not tied to it
I don't mind this explanation - I think the reason GIC has the enum is because the domain xml <gic... > will allow an optional version attribute that has possible values of 2 or 3. Since the vmcoreinfo only cares about supported or not and there's no other attribute, then I think we're good as you've written this (my opinion may not be held by all others in the group and I reserve the right to have it changed ;-)). My question is should the <features> be used for other things and how many features have been added that we missed also adding a domcap for that an up the stack consumer could use? One example that comes to mind because I'm working on it now, I'd 'reuse' this code to do something similar for <genid/> - which is actually a device in qemu terms, but it's either supported or not. It's not a domain <features...> element, but rather a more general metadata element. Likewise, if we consider IOThreads a domain capability - should we report it too as supported or not based on capability? I'm sure there's others, but those come to mind as two that I
docs/formatdomaincaps.html.in | 5 +++++ docs/schemas/domaincaps.rng | 7 +++++++ src/conf/domain_capabilities.c | 2 ++ src/conf/domain_capabilities.h | 1 + src/qemu/qemu_capabilities.c | 3 +++ 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 + tests/domaincapsschemadata/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 + tests/domaincapsschemadata/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 + tests/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 + tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml | 1 + 30 files changed, 43 insertions(+)
diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in index 6bfcaf61c..acdc1cf8a 100644 --- a/docs/formatdomaincaps.html.in +++ b/docs/formatdomaincaps.html.in @@ -417,6 +417,7 @@ <value>3</value> </enum> </gic> + <vmcoreinfo supported='yes'/> </features> </domainCapabilities> </pre> @@ -441,5 +442,9 @@ <code>gic</code> element.</dd> </dl>
+ <h4><a id="elementsvmcoreinfo">vmcoreinfo</a></h4> + + <p>Reports whether the vmcoreinfo feature can be enabled</p>
s/enabled/endabled./
+ </body> </html>
With that small adjustment, consider this: Reviewed-by: John Ferlan <jferlan@redhat.com> John Again, please wait for 4.4.0 before pushing... Also a question, did you hand generate the bhyve and libxl changes (other than the libxl*-usb.xml)? When reusing this for genid, I used the VIR_TEST_REGENERATE_OUTPUT=1 which generated most of the .xml output, but a few didn't show so I'm curious mostly.
On 04/27/2018 11:09 AM, John Ferlan wrote:
On 04/17/2018 02:40 PM, Cole Robinson wrote:
Report domaincaps <features><vmcoreinfo supported='yes'/> if the guest config accepts <features><vmcoreinfo state='on'/>
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- This bucks the domaincapabilities trend of always having a child enum if supported='yes'. Following that trend we would give us this XML when vmcoreinfo is supported:
<vmcoreinfo supported='yes'> <enum name='state'> <value>on</value> <value>off</value> </enum> </vmcoreinfo>
Which is verbose but fine. But it's unclear what we do in the case when vmcoreinfo isn't supported... do we do:
<vmcoreinfo supported='no'/>
which may not be entirely accurate because the code will still accept <vmcoreinfo enabled='off'/>. Or do we do:
<vmcoreinfo supported='yes'> <enum name='state'> <value>off</value> </enum> </vmcoreinfo>
Which is weird IMO. I'm not certain what the semantics of 'supported' are meant to be so I went with this minimal option but I'm not tied to it
I don't mind this explanation - I think the reason GIC has the enum is because the domain xml <gic... > will allow an optional version attribute that has possible values of 2 or 3. Since the vmcoreinfo only cares about supported or not and there's no other attribute, then I think we're good as you've written this (my opinion may not be held by all others in the group and I reserve the right to have it changed ;-)).
My question is should the <features> be used for other things and how many features have been added that we missed also adding a domcap for that an up the stack consumer could use?
One example that comes to mind because I'm working on it now, I'd 'reuse' this code to do something similar for <genid/> - which is actually a device in qemu terms, but it's either supported or not. It's not a domain <features...> element, but rather a more general metadata element.
Likewise, if we consider IOThreads a domain capability - should we report it too as supported or not based on capability? I'm sure there's others, but those come to mind as two that I
docs/formatdomaincaps.html.in | 5 +++++ docs/schemas/domaincaps.rng | 7 +++++++ src/conf/domain_capabilities.c | 2 ++ src/conf/domain_capabilities.h | 1 + src/qemu/qemu_capabilities.c | 3 +++ 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 + tests/domaincapsschemadata/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 + tests/domaincapsschemadata/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 + tests/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 + tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml | 1 + 30 files changed, 43 insertions(+)
diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in index 6bfcaf61c..acdc1cf8a 100644 --- a/docs/formatdomaincaps.html.in +++ b/docs/formatdomaincaps.html.in @@ -417,6 +417,7 @@ <value>3</value> </enum> </gic> + <vmcoreinfo supported='yes'/> </features> </domainCapabilities> </pre> @@ -441,5 +442,9 @@ <code>gic</code> element.</dd> </dl>
+ <h4><a id="elementsvmcoreinfo">vmcoreinfo</a></h4> + + <p>Reports whether the vmcoreinfo feature can be enabled</p>
s/enabled/endabled./
+ </body> </html>
With that small adjustment, consider this:
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
Again, please wait for 4.4.0 before pushing...
Also a question, did you hand generate the bhyve and libxl changes (other than the libxl*-usb.xml)? When reusing this for genid, I used the VIR_TEST_REGENERATE_OUTPUT=1 which generated most of the .xml output, but a few didn't show so I'm curious mostly.
Thanks, I made your suggested changes and pushed. And yeah I had to hand edit the those files, hopefully it's correct. Looks like Martin's latest patch missed doing the same... would be nice to figure out a way around it. Perhaps adjusting the test cases to report 'skip' if the HV isn't compiled it, that should help it stick out in the test output a bit more - Cole
participants (2)
-
Cole Robinson -
John Ferlan