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@xxxxxxxxxx> >> --- >> 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@xxxxxxxxxx> > > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
