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

[Cole Robinson]

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 at 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 at 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