Re: [libvirt] [PATCHv2 10/11] Introduce QEMU_CAPS_DEVICE_VIRTIO_REVISION

On 08/08/2016 12:35 PM, Ján Tomko wrote: > Check whether the disable-legacy property is present on the following > devices: > virtio-balloon-pci > virtio-blk-pci > virtio-scsi-pci > virtio-net-pci > virtio-gpu-pci > > Assuming that if QEMU knows other virtio devices where this property > is applicable, it will have at least one of these devices. Pretty cool idea to look for the presence of disable-legacy in several virtio devices in case the binary has been built without one of them. When I was doing a similar check, I semi-randomly picked "disable-modern", but only checked it on virtio-net. Although in practice that will probably work just as well, I like your idea better.

I'm curious though - did you have a reason for checking for disable-legacy rather than disable-modern? Or was your choice also semi-random?

> Added in QEMU by: > commit e266d421490e0ae83044bbebb209b2d3650c0ba6 > virtio-pci: add flags to enable/disable legacy/modern > --- > src/qemu/qemu_capabilities.c | 6 ++++++ > src/qemu/qemu_capabilities.h | 1 + > tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml | 1 + > tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml | 1 + > tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml | 1 + > tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml | 1 + > tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 1 + > tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 1 + > tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 + > 9 files changed, 14 insertions(+) > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 43e3ea7..340691a 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -340,6 +340,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, > "display", /* 230 */ > "intel-iommu", > "smm", > + "virtio-revision", I had thought about naming my capability flag based on what I used it for as well, but decided it would be better to make the name of the capability as close to the actual option name as possible, to avoid confusion and allow re-use.

Maybe an even better idea would be to make capabilities named "virtio-disable-modern" and "virtio-disable-legacy" set directly based on the presence of the options, then create a synthetic capability named virtio-revision that would be set if both of those others were true (you might find that you need to add more checks in the future, e.g. to disable it for certain interim versions of qemu that had bugs in virtio-1.0)