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

[Laine Stump]

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