Re: [PATCHv4 10/11] Introduce QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.


[1] https://www.redhat.com/archives/libvir-list/2016-August/msg00784.html
-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]