Re: [PATCHv4 10/11] Introduce QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY

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

 



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.

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