[virt-tools-list] [PATCH] virtinst: add <vmcoreinfo/> feature support

[Marc-André Lureau]

Hi

On Wed, Feb 21, 2018 at 4:08 PM, Cole Robinson <crobinso at redhat.com> wrote:
> On 02/20/2018 02:30 PM, marcandre.lureau at redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau at redhat.com>
>>
>> The <vmcoreinfo> feature allows a guest to provide debug details when
>> producing dump. It's useful in particular for Linux guests with KASLR
>> enabled, as otherwise the dump are difficult to analyze.
>>
>
> Thanks for the patch, but, ugh I wish this didn't end up in libvirt as a
> boolean property, there's a reason things are generally tristate these
> days, since there's no way to distinguish between 'use hypervisor
> default' and 'explicitly disable this feature'. What if a future version
> of qemu or machine type defaults to vmcoreinfo=on, how does the user
> request libvirt disable that setting?
>

It translates to -device vmcoreinfo, so you have no other way but to
rely on -nodefaults at qemu level to disable it if some day it is
added by default.

So it will probably have to be specified explicitly all the time by
the domain XML.

Or I would like to know what else we could do..

> Maybe in this particular case it's minor but tristate is much preferred
> IMO. It's probably not too late to change at the libvirt level and
> convert <vmcoreinfo/> to <vmcoreinfo enabled='on'/>, the only reason we
> never did that with preexisting boolean properties is that they were
> around long enough that apps were kinda dependent on it, for reading the
> XML at least.

I also think it can be changed to support the tristate later on, if it
makes sense.

>
>> This patch adds virt-install support for vmcoreinfo domain
>> feature. Whenever the host libvirt/qemu is recent enough, and the VM
>> is x86 or arm-virt, we can assume <vmcoreinfo/> is supported and
>> enable it safely by default (unless --feature vmcoreinfo=on/off is
>> given, or changed via API)
>>
>
> Better I think to split this patch in two parts, the xml/cli wiring
> which is straightforward, the 'adding it as default' which is always
> more interesting to discuss.
>
> My two questions are: 1) are there any downsides to enabling this, 2) if
> we should enable it by default why isn't it enabled by default in qemu,
> maybe tied to new machine types

I agree it would be nice to enable it by default on new machine types.
But since it's a -device, it should be removed anyway with
-nodefaults. Hence setting it should be higher up the stack, when
creating a domain XML.

>
>> Note: I am not quite sure if the tests update require
>> compare_check=support.SUPPORT_CONN_VMCOREINFO has was done for vmport
>> in commit 2d572e02bd619bbbb460f9db3bd671147600926d.
>>
>
> They do, so that there aren't test failures on non-latest libvirt. For
> example the test suite now shows failures with stock f27 packages.

ok

>
> The code itself looks fine though
>

thanks