Re: [libvirt] [PATCH 2/5] Change virPCIDeviceAddress to virDevicePCIAddress
On Sun, Apr 03, 2016 at 10:18:16PM -0400, Laine Stump wrote:
On 04/03/2016 03:27 PM, Martin Kletzander wrote:
We had both and the only difference was that the latter also included
information about multifunction setting. The problem with that was that
we couldn't use functions made for only one of the structs (e.g.
parsing). To consolidate those two structs, move it to virpci.h and
include that in domain_conf.h.
This has annoyed me for a long time, but as usual I just ignored it. I
haven't looked further in your patches to see why you needed to do this
now, but I think I would have preferred it to be solved by leaving
virPCIDeviceAddress as it is, and including one as a member of
virDevicePCIAddress (or if that was too ugly, then naming the new single
struct as virPCIDeviceAddress instead of virDevicePCIAddress, since it's
defined in virpci.h). (I would prefer to not have "multifunction" in the
struct used by virpci.c etc because they don't use that, so it could
confuse people in those usages who might mistakenly believe they should
set it one way or the other; then eventually we would end up with a
bunch of cargo cult settings of multifunction that nobody understood,
but everybody thought were essential).
I'm totally OK with the name being virPCIDeviceAddress instead. But
about (not) having a multifunction there... I'd say if that confuses
someone, then the problem lies somewhere else.
In the end it's eliminating a near duplicate struct, which is a net
positive no matter what the name, so I can give it a soft ACK - ie if
someone else objects then I would be inclined to waffle on my ACK :-)
(But again, I really wonder if this is something needed right before a
release...)
As I said in the cover letter, we don't. The first patch in the series
is enough to fix everything. And it is the version that would be in if
I didn't want to bother with test. However because someone else didn't,
then this bit me in the butt :-/
Signed-off-by: Martin Kletzander
---
src/conf/device_conf.h| 11 +
src/conf/node_device_conf.c | 2 +-
src/conf/node_device_conf.h | 6 +--
src/libvirt_private.syms | 10 ++--
src/network/bridge_driver.c | 4 +-
src/node_device/node_device_linux_sysfs.c | 6 +--
src/util/virhostdev.c | 12 ++---
src/util/virnetdev.c | 4 +-
src/util/virnetdev.h | 2 +-
src/util/virpci.c | 80 +++
src/util/virpci.h | 29 +--
11 files changed, 79 insertions(+), 87 deletions(-)
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index 85ce40f6831e..3fe259cb8916 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -31,6 +31,7 @@
# include "virutil.h"
# include "virthread.h"
# include "virbuffer.h"
+# include "virpci.h"
typedef enum {
VIR_INTERFACE_STATE_UNKNOWN = 1,
@@ -45,16 +46,6 @@ typedef enum {
VIR_ENUM_DECL(virInterfaceState)
-typedef struct _virDevicePCIAddress virDevicePCIAddress;
-typedef virDevicePCIAddress *virDevicePCIAddressPtr;
-struct _virDevicePCIAddress {
-unsigned int domain;
-unsigned int bus;
-unsigned int slot;
-unsigned int function;
-int multi; /* virTristateSwitch */
-};
-
typedef struct _virInterfaceLink virInterfaceLink;
typedef virInterfaceLink *virInterfaceLinkPtr;
struct _virInterfaceLink {
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index a76f785eddc0..3e9c821762eb 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -1142,7 +1142,7 @@ virNodeDevCapPCIDevIommuGroupParseXML(xmlXPathContextPtr
ctxt,
char *numberStr = NULL;
int nAddrNodes, ret = -1;
size_t i;
-virPCIDeviceAddressPtr pciAddr = NULL;
+virDevicePCIAddressPtr pciAddr = NULL;
ctxt->node = iommuGroupNode;
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index be6dd5eb4ec1..9c9d942dd506 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -109,12 +109,12 @@ typedef struct _virNodeDevCapData {
unsigned int class;
char *product_name;
char *vendor_name;
-virPCIDeviceAddressPtr physical_function;
-virPCIDeviceAddressPtr *virtual_functions;
+virDevicePCIAddressPtr physical_function;
+virDevicePCIAddressPtr *virtual_functions;
size_t num_virtual_functions;
unsigned int max_virtual_functions;
unsigned int flags;
-virPCIDeviceAddressPtr *iommuGroupDevices;
+virDevicePCIAddressPtr *iommuGroupDevices;
size_t nIommuGroupDevices;
unsigned int iommuGroupNumber;
int numa_node;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 684f06cd4f16..8bb901568079 1
Re: [libvirt] [PATCH 2/5] Change virPCIDeviceAddress to virDevicePCIAddress
On 04/03/2016 03:27 PM, Martin Kletzander wrote:
We had both and the only difference was that the latter also included
information about multifunction setting. The problem with that was that
we couldn't use functions made for only one of the structs (e.g.
parsing). To consolidate those two structs, move it to virpci.h and
include that in domain_conf.h.
This has annoyed me for a long time, but as usual I just ignored it. I
haven't looked further in your patches to see why you needed to do this
now, but I think I would have preferred it to be solved by leaving
virPCIDeviceAddress as it is, and including one as a member of
virDevicePCIAddress (or if that was too ugly, then naming the new single
struct as virPCIDeviceAddress instead of virDevicePCIAddress, since it's
defined in virpci.h). (I would prefer to not have "multifunction" in the
struct used by virpci.c etc because they don't use that, so it could
confuse people in those usages who might mistakenly believe they should
set it one way or the other; then eventually we would end up with a
bunch of cargo cult settings of multifunction that nobody understood,
but everybody thought were essential).
In the end it's eliminating a near duplicate struct, which is a net
positive no matter what the name, so I can give it a soft ACK - ie if
someone else objects then I would be inclined to waffle on my ACK :-)
(But again, I really wonder if this is something needed right before a
release...)
Signed-off-by: Martin Kletzander
---
src/conf/device_conf.h| 11 +
src/conf/node_device_conf.c | 2 +-
src/conf/node_device_conf.h | 6 +--
src/libvirt_private.syms | 10 ++--
src/network/bridge_driver.c | 4 +-
src/node_device/node_device_linux_sysfs.c | 6 +--
src/util/virhostdev.c | 12 ++---
src/util/virnetdev.c | 4 +-
src/util/virnetdev.h | 2 +-
src/util/virpci.c | 80 +++
src/util/virpci.h | 29 +--
11 files changed, 79 insertions(+), 87 deletions(-)
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index 85ce40f6831e..3fe259cb8916 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -31,6 +31,7 @@
# include "virutil.h"
# include "virthread.h"
# include "virbuffer.h"
+# include "virpci.h"
typedef enum {
VIR_INTERFACE_STATE_UNKNOWN = 1,
@@ -45,16 +46,6 @@ typedef enum {
VIR_ENUM_DECL(virInterfaceState)
-typedef struct _virDevicePCIAddress virDevicePCIAddress;
-typedef virDevicePCIAddress *virDevicePCIAddressPtr;
-struct _virDevicePCIAddress {
-unsigned int domain;
-unsigned int bus;
-unsigned int slot;
-unsigned int function;
-int multi; /* virTristateSwitch */
-};
-
typedef struct _virInterfaceLink virInterfaceLink;
typedef virInterfaceLink *virInterfaceLinkPtr;
struct _virInterfaceLink {
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index a76f785eddc0..3e9c821762eb 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -1142,7 +1142,7 @@ virNodeDevCapPCIDevIommuGroupParseXML(xmlXPathContextPtr
ctxt,
char *numberStr = NULL;
int nAddrNodes, ret = -1;
size_t i;
-virPCIDeviceAddressPtr pciAddr = NULL;
+virDevicePCIAddressPtr pciAddr = NULL;
ctxt->node = iommuGroupNode;
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index be6dd5eb4ec1..9c9d942dd506 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -109,12 +109,12 @@ typedef struct _virNodeDevCapData {
unsigned int class;
char *product_name;
char *vendor_name;
-virPCIDeviceAddressPtr physical_function;
-virPCIDeviceAddressPtr *virtual_functions;
+virDevicePCIAddressPtr physical_function;
+virDevicePCIAddressPtr *virtual_functions;
size_t num_virtual_functions;
unsigned int max_virtual_functions;
unsigned int flags;
-virPCIDeviceAddressPtr *iommuGroupDevices;
+virDevicePCIAddressPtr *iommuGroupDevices;
size_t nIommuGroupDevices;
unsigned int iommuGroupNumber;
int numa_node;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 684f06cd4f16..8bb901568079 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1966,11 +1966,11 @@ virObjectUnref;
# util/virpci.h
-virPCIDeviceAddressGetIOMMUGroupAddresses;
-virPCIDeviceAddressGetIOMMUGroupNum;
-virPCIDeviceAddressGetSysfsFile;
-virPCIDeviceAddressIOMMUGroupIterate;
-virPCIDeviceAddressParse;
+virDevicePCIAddressGetIOMMUGroupAddresses;
+virDevicePCIAddressGetIOMMUGroupNum;
+virDevicePCIAddressGetSysfsFile;
+virDevicePCIAddressIOMMUGroupIterate;
+virDevicePCIAddressPa
