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

[Cole Robinson]

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?

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.

> 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

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

The code itself looks fine though

Thanks,
Cole