Re: [libvirt] [PATCHv4 10/11] Introduce QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY

On Thu, 2016-08-11 at 13:57 +0200, 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-serial-pci > virtio-9p-pci > virtio-net-pci > virtio-rng-pci > virtio-gpu-pci > virtio-input-host-pci > virtio-keyboard-pci > virtio-mouse-pci > virtio-tablet-pci > > Assuming that if QEMU knows other virtio devices where this property > is applicable, it will have at least one of these devices. > > Added in QEMU by: > commit e266d421490e0ae83044bbebb209b2d3650c0ba6 > virtio-pci: add flags to enable/disable legacy/modern I looked at this patch because it's a requirement for Laine's PCIe series. I'll just point out a couple things; I see there have been a few comments about the design of the interface that you'll need to address, so I don't think it's very useful to look at the whole series before you've had a chance to do so. > +struct virQEMUCapsPropObjects { > + const char *prop; > + int flag; > + const char **objects; > +}; > + > +static const char *virQEMUCapsVirtioPCIDisableLegacyObjects[] = { > + "virtio-balloon-pci", > + "virtio-blk-pci", > + "virtio-scsi-pci", > + "virtio-serial-pci", > + "virtio-9p-pci", > + "virtio-net-pci", > + "virtio-rng-pci", > + "virtio-gpu-pci", > + "virtio-input-host-pci", > + "virtio-keyboard-pci", > + "virtio-mouse-pci", > + "virtio-tablet-pci", > + NULL > +}; > + > +static struct virQEMUCapsPropObjects virQEMUCapsPropObjects[] = { Please, don't :) Use something like virQEMUCapsPropTypeObjects (to mirror the existing virQEMUCapsObjectTypeProps), or virQEMUCapsPropObjectsType, or anything really - just make sure the name of the type and the name of the variable containing a bunch of instances of said type are not the same. > static void > +virQEMUCapsProcessProps(virQEMUCapsPtr qemuCaps, > + size_t nprops, > + struct virQEMUCapsPropObjects *props, > + const char *object, > + size_t nvalues, > + char *const*values) > +{ > + size_t i, j; > + > + for (i = 0; i < nprops; i++) { > + if (virQEMUCapsGet(qemuCaps, props[i].flag)) > + continue; > + > + for (j = 0; j < nvalues; j++) { > + if (STREQ(values[j], props[i].prop)) { > + if (virStringArrayHasString((char **)props[i].objects, object)) Rather than casting a const char ** to char **, which happens in other places as well, it would be IMHO much better to make virStringArrayHasString() accept a const char ** as the first argument. And guess what? I just posted a patch[1] that does exactly that :) Everything else looks good.