Hi Laine,
Thanks for the comments. Please find my reply inline.
On Tue, Mar 24, 2015 at 10:03 PM, Laine Stump <laine@xxxxxxxxx> wrote:
> 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 agree with you. The virInterfaceState and virInterfaceState being the only
virInterface* definitions and having no functions starting with virInterface*,
I decided to move to virnetdev.h file, given that is the only
utils/*.c/h file using
them. That was the reason behind choosing this file.
>
> 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?
Yes. This patch is supposed to be the part of such a series. I
am still working on them. Need some more time posting them.
>
> (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.)
I was thinking for changing the virDomainObj to virObject instead, as per Dan's
suggestion in IRC. That is still work a in progress.
>
>>
>> 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?
Yes. I was referring to that.
>
>>
>> Signed-off-by: Shivaprasad G Bhat <sbhat@xxxxxxxxxxxxxxxxxx>
>> ---
>> 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.
I'll move them downwards to make it clean.
>
>> typedef struct _virNetDevRxFilter virNetDevRxFilter;
>> typedef virNetDevRxFilter *virNetDevRxFilterPtr;
>> struct _virNetDevRxFilter {
>>
>> --
>> libvir-list mailing list
>> libvir-list@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>
I am sending v2 with the code movement as suggested.
Thanks,
Shiva
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list