[dpdk-dev,v2,07/13] net/avp: fix errors in exported headers
Checks
Commit Message
This commit addresses several errors related to missing includes such as:
In file included from /tmp/check-includes.sh.15315.c:1:0:
build/include/rte_avp_fifo.h:77:22: error: 'struct rte_avp_fifo' declared
inside parameter list [-Werror]
[...]
build/include/rte_avp_fifo.h: In function 'avp_fifo_init':
build/include/rte_avp_fifo.h:81:3: error: implicit declaration of function
'rte_panic' [-Werror=implicit-function-declaration]
[...]
build/include/rte_avp_fifo.h:83:6: error: dereferencing pointer to
incomplete type
[...]
build/include/rte_avp_fifo.h:109:2: error: implicit declaration of
function 'rte_wmb' [-Werror=implicit-function-declaration]
[...]
In file included from /tmp/check-includes.sh.15315.c:1:0:
build/include/rte_avp_common.h:104:2: error: unknown type name 'uint64_t'
[...]
build/include/rte_avp_common.h:386:15: error: 'ETHER_ADDR_LEN' undeclared
here (not in a function)
[...]
It addresses errors with strict compilation flags:
In file included from /tmp/check-includes.sh.15315.c:1:0:
build/include/rte_avp_common.h:122:3: error: ISO C99 doesn't support
unnamed structs/unions [-Werror=pedantic]
[...]
build/include/rte_avp_common.h:136:17: error: ISO C forbids zero-size
array 'buffer' [-Werror=pedantic]
[...]
And also adds C++ awareness to both header files.
Fixes: 8e680655e205 ("net/avp: add public header files")
Cc: Allain Legacy <allain.legacy@windriver.com>
Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
drivers/net/avp/rte_avp_common.h | 17 ++++++++++++++++-
drivers/net/avp/rte_avp_fifo.h | 12 ++++++++++++
2 files changed, 28 insertions(+), 1 deletion(-)
Comments
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Tuesday, April 25, 2017 4:30 AM
<...>
>
> +#include <stdint.h>
> #ifdef __KERNEL__
> #include <linux/if.h>
> +#else
> +#include <rte_common.h>
> +#include <rte_memory.h>
> +#include <rte_ether.h>
> +#include <rte_atomic.h>
> +#endif
I compiled this in our environment and found a couple of additional issues. I apologize... I should have done that on the first pass. It should actually look like this to handle both userspace and kernel compiles:
#ifdef __KERNEL__
#include <linux/if.h>
#define RTE_STD_C11
#else
#include <stdint.h>
#include <rte_common.h>
#include <rte_memory.h>
#include <rte_ether.h>
#include <rte_atomic.h>
#endif
1) stdint.h needs to be moved in to the #else, and
2) RTE_STD_C11 needs to be included in the #ifdef __KERNEL__.
<..>
> diff --git a/drivers/net/avp/rte_avp_fifo.h b/drivers/net/avp/rte_avp_fifo.h
> index 8262e4f..a0a37eb 100644
> --- a/drivers/net/avp/rte_avp_fifo.h
> +++ b/drivers/net/avp/rte_avp_fifo.h
> @@ -57,6 +57,12 @@
> #ifndef _RTE_AVP_FIFO_H_
> #define _RTE_AVP_FIFO_H_
>
> +#include <rte_avp_common.h>
Would you mind changing the brackets (<>) to quotes ("") since this is a local include file?
#include "rte_avp_common.h"
On Tue, Apr 25, 2017 at 12:31:56PM +0000, Legacy, Allain wrote:
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Tuesday, April 25, 2017 4:30 AM
> <...>
> >
> > +#include <stdint.h>
> > #ifdef __KERNEL__
> > #include <linux/if.h>
> > +#else
> > +#include <rte_common.h>
> > +#include <rte_memory.h>
> > +#include <rte_ether.h>
> > +#include <rte_atomic.h>
> > +#endif
>
> I compiled this in our environment and found a couple of additional issues. I apologize... I should have done that on the first pass. It should actually look like this to handle both userspace and kernel compiles:
>
> #ifdef __KERNEL__
> #include <linux/if.h>
> #define RTE_STD_C11
> #else
> #include <stdint.h>
> #include <rte_common.h>
> #include <rte_memory.h>
> #include <rte_ether.h>
> #include <rte_atomic.h>
> #endif
>
> 1) stdint.h needs to be moved in to the #else, and
OK, will update.
> 2) RTE_STD_C11 needs to be included in the #ifdef __KERNEL__.
Missed that one, however I suggest either:
#ifndef __KERNEL__ around RTE_STD_C11
or using __extension__ directly. Which do you prefer?
By the way, is the kernel module that depends on rte_avp_common.h available
somewhere to validate compilation against it?
> <..>
> > diff --git a/drivers/net/avp/rte_avp_fifo.h b/drivers/net/avp/rte_avp_fifo.h
> > index 8262e4f..a0a37eb 100644
> > --- a/drivers/net/avp/rte_avp_fifo.h
> > +++ b/drivers/net/avp/rte_avp_fifo.h
> > @@ -57,6 +57,12 @@
> > #ifndef _RTE_AVP_FIFO_H_
> > #define _RTE_AVP_FIFO_H_
> >
> > +#include <rte_avp_common.h>
>
> Would you mind changing the brackets (<>) to quotes ("") since this is a local include file?
>
> #include "rte_avp_common.h"
I will update it.
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Tuesday, April 25, 2017 8:50 AM
<...>
> > 2) RTE_STD_C11 needs to be included in the #ifdef __KERNEL__.
>
> Missed that one, however I suggest either:
>
> #ifndef __KERNEL__ around RTE_STD_C11
>
> or using __extension__ directly. Which do you prefer?
I would prefer if it was done as it is done in rte_kni_common.h to provide consistency with other similar files. Like this:
#ifdef __KERNEL__
#include <linux/if.h>
#define RTE_STD_C11
#else
#include <rte_common.h>
#endif
...but if you disagree then I prefer the #ifndef __KERNEL__ option.
>
> By the way, is the kernel module that depends on rte_avp_common.h
> available somewhere to validate compilation against it?
There is an older version of the module available on github, but it has not been updated since the AVP driver has been included in the DPDK. Since the AVP directory and files were significantly changed in order to meet the requirements of the DPDK it won't be much use to you. Until we can update it please make sure both Matt Peters and I are CC'd on the patch requests and we'll confirm compilation as quickly as possible.
> > Would you mind changing the brackets (<>) to quotes ("") since this is a
> local include file?
> >
> > #include "rte_avp_common.h"
>
> I will update it.
Thank you.
On Tue, Apr 25, 2017 at 01:00:46PM +0000, Legacy, Allain wrote:
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Tuesday, April 25, 2017 8:50 AM
> <...>
> > > 2) RTE_STD_C11 needs to be included in the #ifdef __KERNEL__.
> >
> > Missed that one, however I suggest either:
> >
> > #ifndef __KERNEL__ around RTE_STD_C11
> >
> > or using __extension__ directly. Which do you prefer?
>
> I would prefer if it was done as it is done in rte_kni_common.h to provide consistency with other similar files. Like this:
>
> #ifdef __KERNEL__
> #include <linux/if.h>
> #define RTE_STD_C11
> #else
> #include <rte_common.h>
> #endif
>
> ...but if you disagree then I prefer the #ifndef __KERNEL__ option.
You're right in fact, I did not remember that was the method used for
KNI. Let's keep your suggestion.
> >
> > By the way, is the kernel module that depends on rte_avp_common.h
> > available somewhere to validate compilation against it?
>
> There is an older version of the module available on github, but it has not been updated since the AVP driver has been included in the DPDK. Since the AVP directory and files were significantly changed in order to meet the requirements of the DPDK it won't be much use to you. Until we can update it please make sure both Matt Peters and I are CC'd on the patch requests and we'll confirm compilation as quickly as possible.
>
>
> > > Would you mind changing the brackets (<>) to quotes ("") since this is a
> > local include file?
> > >
> > > #include "rte_avp_common.h"
> >
> > I will update it.
>
>
> Thank you.
Can I add your acked-by line directly assuming all the above is done as
described?
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Tuesday, April 25, 2017 10:49 AM
<...>
> > Thank you.
>
> Can I add your acked-by line directly assuming all the above is done as
> described?
Yes.
@@ -57,8 +57,18 @@
#ifndef _RTE_AVP_COMMON_H_
#define _RTE_AVP_COMMON_H_
+#include <stdint.h>
#ifdef __KERNEL__
#include <linux/if.h>
+#else
+#include <rte_common.h>
+#include <rte_memory.h>
+#include <rte_ether.h>
+#include <rte_atomic.h>
+#endif
+
+#ifdef __cplusplus
+extern "C" {
#endif
/**
@@ -115,6 +125,7 @@ struct rte_avp_device_config {
*/
struct rte_avp_request {
uint32_t req_id; /**< Request id */
+ RTE_STD_C11
union {
uint32_t new_mtu; /**< New MTU */
uint8_t if_up; /**< 1: interface up, 0: interface down */
@@ -133,7 +144,7 @@ struct rte_avp_fifo {
volatile unsigned int read; /**< Next position to be read */
unsigned int len; /**< Circular buffer length */
unsigned int elem_size; /**< Pointer size - for 32/64 bit OS */
- void *volatile buffer[0]; /**< The buffer contains mbuf pointers */
+ void *volatile buffer[]; /**< The buffer contains mbuf pointers */
};
@@ -413,4 +424,8 @@ struct rte_avp_device_info {
#define RTE_AVP_IOCTL_RELEASE _IOWR(0, 3, struct rte_avp_device_info)
#define RTE_AVP_IOCTL_QUERY _IOWR(0, 4, struct rte_avp_device_config)
+#ifdef __cplusplus
+}
+#endif
+
#endif /* _RTE_AVP_COMMON_H_ */
@@ -57,6 +57,12 @@
#ifndef _RTE_AVP_FIFO_H_
#define _RTE_AVP_FIFO_H_
+#include <rte_avp_common.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
#ifdef __KERNEL__
/* Write memory barrier for kernel compiles */
#define AVP_WMB() smp_wmb()
@@ -70,6 +76,8 @@
#endif
#ifndef __KERNEL__
+#include <rte_debug.h>
+
/**
* Initializes the avp fifo structure
*/
@@ -154,4 +162,8 @@ avp_fifo_free_count(struct rte_avp_fifo *fifo)
return (fifo->read - fifo->write - 1) & (fifo->len - 1);
}
+#ifdef __cplusplus
+}
+#endif
+
#endif /* _RTE_AVP_FIFO_H_ */