<html> <head> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> <div class="moz-cite-prefix">On 05/05/2014 03:07 PM, Ján Tomko wrote: </div> <blockquote cite="mid:53677EF3.1080508@redhat.com" type="cite"> <pre wrap="">On 05/03/2014 06:31 PM, Roman Bogorodskiy wrote: </pre> <blockquote type="cite"> <pre wrap="">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 </pre> </blockquote> <pre wrap=""> I would drop the 'Domain', to make the prefix match the file.</pre> </blockquote> 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! :-) <blockquote cite="mid:53677EF3.1080508@redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap=""> Also, extract qemuDomainPCIAddressAsString function and rename it into virPCIDeviceAddressAsString. </pre> </blockquote> <pre wrap=""> If that was moved into the second patch, this one would touch two less files (unless I missed something).</pre> </blockquote> 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... <blockquote cite="mid:53677EF3.1080508@redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">--- 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(-) </pre> </blockquote> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">@@ -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); </pre> </blockquote> <pre wrap=""> Indentation is off here. </pre> <blockquote type="cite"> <pre wrap=""> - if (!(addrStr = qemuDomainPCIAddressAsString(&dev->addr.pci))) + if (!(addrStr = virPCIDeviceAddressAsString(&dev->addr.pci))) goto cleanup; if (dev->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { </pre> </blockquote> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">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" </pre> </blockquote> <pre wrap=""> 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 </pre> <fieldset class="mimeAttachmentHeader"></fieldset> <pre wrap="">-- libvir-list mailing list <a class="moz-txt-link-abbreviated" href="mailto:libvir-list@redhat.com">libvir-list@redhat.com</a> <a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/libvir-list">https://www.redhat.com/mailman/listinfo/libvir-list</a></pre> </blockquote> </body> </html>