[libvirt] [PATCH v3 1/3] qemu: extract PCI handling structs

[Laine Stump]

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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140505/5389f882/attachment-0001.htm>