On 05/05/2014 03:07 PM, Ján Tomko wrote:
On 05/03/2014 06:31 PM, Roman Bogorodskiy wrote:

Extract PCI handling related structs that could be shared
with other drivers.

List of structs moved to virpci.h and new names:

 qemuDomainPCIAddressBus -> virDomainPCIAddressBus
 qemuDomainPCIAddressBusPtr -> virDomainPCIAddressBusPtr
 _qemuDomainPCIAddressSet -> virDomainPCIAddressSet
 qemuDomainPCIAddressSetPtr -> virDomainPCIAddressSetPtr
 qemuDomainPCIConnectFlags -> virDomainPCIConnectFlags

I would drop the 'Domain', to make the prefix match the file.

I was thinking about that and came to a different opinion. The functions that are currently in virpci.c are dealing with manipulating and reporting about PCI devices on the *host* (reading and writing sysfs files to attach and detach drivers, determining the list of virtual functions for an SRIOV physical function, etc), while these functions that Roman is moving are only concerned with managing the allocation of PCI addresses to devices in a domain.

Because of that, I think it's reasonable (a good idea really) to keep "Domain" in the function names.

Beyond that, I was going to say that I think these functions belong in their own file, *not* virpci.c (and maybe we even want to rename virpci.c to virhostpci.c or something). I think it's *essential* that the two sets of functions are separated from each other, since what is in virpci.c is Linux-specific, but the virDomainPCI... functions should be host-agnostic.


BTW, thanks for caving in to our suggestion so willingly, Roman! :-)




Also, extract qemuDomainPCIAddressAsString function and rename
it into virPCIDeviceAddressAsString.

If that was moved into the second patch, this one would touch two less files
(unless I missed something).

One thing that's bothered me (and I *almost* did something about it the last time I sent patches that touched PCI code) is that there are two *almost* identical data structures in the code:

virPCIDeviceAddress  (in virpci.h)
   has domain, bus, slot, function
virDevicePCIAddress (in device_conf.h)
   has domain, bus, slot, function, _and multifunction_

(there is also _virPCIDevice, but that is only used in virpci.c, and has quite a few more attributes in it)

This leads to places where we assign from one to the other by individual attributes, rather than just assigning the entire object, argument lists that break out the individual attributes instead of passing an object, and confusion when you accidentally use the wrong one.

I'm thinking that virDevicePCIAddress should probably contain a virPCIDeviceAddress (and maybe have a name that is more different), and that virpci.c should have a few functions that format a virPCIDeviceAddress into a char* and a virBufferPtr.

But that's not anything that should hold up this series, just thought I should mention it because it's in the same area...



---
 src/Makefile.am          |   2 +-
 src/libvirt_private.syms |   1 +
 src/qemu/qemu_command.c  | 201 +++++++++++++++++++----------------------------
 src/qemu/qemu_command.h  |  40 +++-------
 src/qemu/qemu_domain.h   |   5 +-
 src/util/virpci.c        |  14 ++++
 src/util/virpci.h        |  48 +++++++++++
 tools/Makefile.am        |   1 +
 8 files changed, 158 insertions(+), 154 deletions(-)


      

        
@@ -2134,10 +2093,10 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs,
      * function is only used for hot-plug, though, and hot-plug is
      * only supported for standard PCI devices, so we can safely use
      * the setting below */
-    qemuDomainPCIConnectFlags flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE |
-                                       QEMU_PCI_CONNECT_TYPE_PCI);
+    virDomainPCIConnectFlags flags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
+                                       VIR_PCI_CONNECT_TYPE_PCI);


      

      
Indentation is off here.



      

        
 
-    if (!(addrStr = qemuDomainPCIAddressAsString(&dev->addr.pci)))
+    if (!(addrStr = virPCIDeviceAddressAsString(&dev->addr.pci)))
         goto cleanup;
 
     if (dev->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {


      

      

      

        
diff --git a/src/util/virpci.h b/src/util/virpci.h
index 20ffe54..21fe261 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -26,6 +26,7 @@
 
 # include "internal.h"
 # include "virobject.h"
+# include "domain_conf.h"


      

      
Hmm, this requires the Makefile changes just to get around the cross-inclusion
syntax check rule, that is probably not a good idea.

It's needed for virDevicePCIAddress and virDomainControllerModelPCI.

We have virDevicePCIAddress (defined in device_conf.h) for guest devices and
virPCIDeviceAddress (here in virpci.h) for host devices.

virDomainControllerModelPCI is needed by the PCIAddressBusSetModel set
function, which fills out the flags and {min,max}Slots and also the model,
but I can't find any function using it.

I think the right thing is to drop the include - change the guest devices to
use the address type defined here in virpci.h and stop storing the model with
the address bus.

Jan



      

      

      

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