Re: [libvirt] [PATCH v3 1/2] cleanup conf/device_conf.h inclusion from util/virnetdev.h

On 03/24/2015 05:57 AM, Shivaprasad G Bhat wrote: > Move the struct and enum definitions from device_conf.h to > virnetdev.h thus avoiding the file inclusion. > > The wrong reference of conf files from util/ is allowed > in Makefile.am for now. The reason being, > The change in Makefile.am for libvirt_util_la_CFLAGS to remove > conf from the utils would require corresponding inclusion in util files > to specify the paths explicitly. This would result in syntax-check failures > (prohibit_cross_inclusion) which need to be worked around until > there are patches reworking the incorrect inclusion. I agree that we should eliminate this #include of something from conf in util (and definitely that is a more egregious problem than calling a platform specific function from conf), but disagree with using this patch as the method of assuring that virNetDevExists() is declared for network_conf.c (so that patch 2/2 of this series compiles). if virnetdev.h is required by something in network_conf.c, then it should be #included by network_conf.c. (yes, I understand that the code movement in this patch makes it necessary to #include virnetdev.h in device_conf.h anyway)

I see that there are a few other cases of *_conf.h files being included from .h files in the util directory. Is this patch perhaps part of a patch series to fix that?

(BTW, the util directory was clean of any #includes from the conf directory until last July, when management of "close callbacks" was moved from the qemu driver into util/virclosecallbacks.h. The problem is that one of the args to the a callback is a virDomainObj. So I think the proper solution to this problem would require making that arg to all the virCloseCallback functions an opaque pointer, show real nature would be known only to the callback function itself.)

> > The syntax-check failure workaround seems dangerous as that might mask some > easily resolvable failures. So dont touch the Makefile.am for now. Resolve > the wrong inclusions in separate patches. Maybe that's what you're referring to here?

> > Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; > --- > src/conf/device_conf.h | 38 +------------------------------------- > src/util/virnetdev.h | 38 +++++++++++++++++++++++++++++++++++++- > 2 files changed, 38 insertions(+), 38 deletions(-) > > diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h > index 7ea90f6..a650189 100644 > --- a/src/conf/device_conf.h > +++ b/src/conf/device_conf.h > @@ -31,19 +31,7 @@ > # include "virutil.h" > # include "virthread.h" > # include "virbuffer.h" > - > -typedef enum { > - VIR_INTERFACE_STATE_UNKNOWN = 1, > - VIR_INTERFACE_STATE_NOT_PRESENT, > - VIR_INTERFACE_STATE_DOWN, > - VIR_INTERFACE_STATE_LOWER_LAYER_DOWN, > - VIR_INTERFACE_STATE_TESTING, > - VIR_INTERFACE_STATE_DORMANT, > - VIR_INTERFACE_STATE_UP, > - VIR_INTERFACE_STATE_LAST > -} virInterfaceState; > - > -VIR_ENUM_DECL(virInterfaceState) > +# include "virnetdev.h" > > typedef struct _virDevicePCIAddress virDevicePCIAddress; > typedef virDevicePCIAddress *virDevicePCIAddressPtr; > @@ -55,30 +43,6 @@ struct _virDevicePCIAddress { > int multi; /* virTristateSwitch */ > }; > > -typedef struct _virInterfaceLink virInterfaceLink; > -typedef virInterfaceLink *virInterfaceLinkPtr; > -struct _virInterfaceLink { > - virInterfaceState state; /* link state */ > - unsigned int speed; /* link speed in Mbits per second */ > -}; > - > -typedef enum { > - VIR_NET_DEV_FEAT_GRXCSUM, > - VIR_NET_DEV_FEAT_GTXCSUM, > - VIR_NET_DEV_FEAT_GSG, > - VIR_NET_DEV_FEAT_GTSO, > - VIR_NET_DEV_FEAT_GGSO, > - VIR_NET_DEV_FEAT_GGRO, > - VIR_NET_DEV_FEAT_LRO, > - VIR_NET_DEV_FEAT_RXVLAN, > - VIR_NET_DEV_FEAT_TXVLAN, > - VIR_NET_DEV_FEAT_NTUPLE, > - VIR_NET_DEV_FEAT_RXHASH, > - VIR_NET_DEV_FEAT_LAST > -} virNetDevFeature; > - > -VIR_ENUM_DECL(virNetDevFeature) > - > int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr); > > int virDevicePCIAddressParseXML(xmlNodePtr node, > diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h > index 856127b..daba0bb 100644 > --- a/src/util/virnetdev.h > +++ b/src/util/virnetdev.h > @@ -30,7 +30,6 @@ > # include "virnetlink.h" > # include "virmacaddr.h" > # include "virpci.h" > -# include "device_conf.h" > > # ifdef HAVE_STRUCT_IFREQ > typedef struct ifreq virIfreq; > @@ -47,6 +46,43 @@ typedef enum { > } virNetDevRxFilterMode; > VIR_ENUM_DECL(virNetDevRxFilterMode) > > +typedef enum { > + VIR_INTERFACE_STATE_UNKNOWN = 1, > + VIR_INTERFACE_STATE_NOT_PRESENT, > + VIR_INTERFACE_STATE_DOWN, > + VIR_INTERFACE_STATE_LOWER_LAYER_DOWN, > + VIR_INTERFACE_STATE_TESTING, > + VIR_INTERFACE_STATE_DORMANT, > + VIR_INTERFACE_STATE_UP, > + VIR_INTERFACE_STATE_LAST > +} virInterfaceState; > + > +VIR_ENUM_DECL(virInterfaceState) > + > +typedef struct _virInterfaceLink virInterfaceLink; > +typedef virInterfaceLink *virInterfaceLinkPtr; > +struct _virInterfaceLink { > + virInterfaceState state; /* link state */ > + unsigned int speed; /* link speed in Mbits per second */ > +}; > + > +typedef enum { > + VIR_NET_DEV_FEAT_GRXCSUM, > + VIR_NET_DEV_FEAT_GTXCSUM, > + VIR_NET_DEV_FEAT_GSG, > + VIR_NET_DEV_FEAT_GTSO, > + VIR_NET_DEV_FEAT_GGSO, > + VIR_NET_DEV_FEAT_GGRO, > + VIR_NET_DEV_FEAT_LRO, > + VIR_NET_DEV_FEAT_RXVLAN, > + VIR_NET_DEV_FEAT_TXVLAN, > + VIR_NET_DEV_FEAT_NTUPLE, > + VIR_NET_DEV_FEAT_RXHASH, > + VIR_NET_DEV_FEAT_LAST > +} virNetDevFeature; > + > +VIR_ENUM_DECL(virNetDevFeature) > + It's a bit odd that you've moved this unrelated stuff in between virNetDevRxFilterMode and virNetDevRxFilter. Those should probably be left together.

> typedef struct _virNetDevRxFilter virNetDevRxFilter; > typedef virNetDevRxFilter *virNetDevRxFilterPtr; > struct _virNetDevRxFilter { > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list >