Re: [libvirt] [PATCH 3/3] domain_capabilities: Report <vmcoreinfo> support

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(a)redhat.com&gt; > --- > 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 @@ > &lt;value&gt;3&lt;/value&gt; > &lt;/enum&gt; > &lt;/gic&gt; > + &lt;vmcoreinfo supported='yes'/&gt; > &lt;/features&gt; > &lt;/domainCapabilities&gt; > </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(a)redhat.com&gt; 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.