[libvirt] [PATCHv4 10/11] Introduce QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY

[Laine Stump]

On 08/16/2016 07:50 AM, Andrea Bolognani wrote:
> 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.

I'll ACK this pending the two changes abologna suggested. If you're 
confident you won't want to change this, but might be delayed in redoing 
the rest of the series, feel free to push this one ahead of the rest, as 
I am using it in my "Use more PCIe less PCI" series, which is mostly ACKed.