Re: [dpdk-stable] [PATCH] eal/headers: explicitly cast void * to type *

Re: [dpdk-stable] [PATCH] eal/headers: explicitly cast void * to type *

[Dmitry Kozlyuk]

On Mon, 11 Jan 2021 17:18:45 -0800, Tyler Retzlaff wrote:
> Explicitly cast void * to type * so that eal headers may be compiled
> as C or C++.

Topic should probably be "eal/windows".

Fixes: e8428a9d89f1 ("eal/windows: add some basic functions and macros")
Cc: stable@dpdk.org

> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
[...] 
> diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
> index bf715896a..c20be29b1 100644
> --- a/lib/librte_ethdev/rte_ethdev_pci.h
> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
> @@ -47,7 +47,7 @@ rte_eth_copy_pci_info(struct rte_eth_dev *eth_dev,
>  
>  static inline int
>  eth_dev_pci_specific_init(struct rte_eth_dev *eth_dev, void *bus_device) {
> -	struct rte_pci_device *pci_dev = bus_device;
> +	struct rte_pci_device *pci_dev = (struct rte_pci_device *)bus_device;
>  
>  	if (!pci_dev)
>  		return -ENODEV;

This is a private header, it's never exposed---why the change is needed (not
that I have a strong opinion, though)?

Re: [dpdk-stable] [PATCH] eal/headers: explicitly cast void * to type *

[Tyler Retzlaff]

On Wed, Jan 13, 2021 at 08:52:55PM +0300, Dmitry Kozlyuk wrote:
> On Mon, 11 Jan 2021 17:18:45 -0800, Tyler Retzlaff wrote:
> > Explicitly cast void * to type * so that eal headers may be compiled
> > as C or C++.
> 
> Topic should probably be "eal/windows".

i'll submit a new rev that changes this, it's not really windows specific
but i guess windows is the only people crazy enough to use c++.

> 
> Fixes: e8428a9d89f1 ("eal/windows: add some basic functions and macros")
> Cc: stable@dpdk.org

yep, will do thanks.

> 
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> [...] 
> > diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
> > index bf715896a..c20be29b1 100644
> > --- a/lib/librte_ethdev/rte_ethdev_pci.h
> > +++ b/lib/librte_ethdev/rte_ethdev_pci.h
> > @@ -47,7 +47,7 @@ rte_eth_copy_pci_info(struct rte_eth_dev *eth_dev,
> >  
> >  static inline int
> >  eth_dev_pci_specific_init(struct rte_eth_dev *eth_dev, void *bus_device) {
> > -	struct rte_pci_device *pci_dev = bus_device;
> > +	struct rte_pci_device *pci_dev = (struct rte_pci_device *)bus_device;
> >  
> >  	if (!pci_dev)
> >  		return -ENODEV;
> 
> This is a private header, it's never exposed---why the change is needed (not
> that I have a strong opinion, though)?

interesting, i'll look into why/how it is being included and confirm. i suppose
the question in the back of my mind is if it is private then why is the header
being installed at all?

Re: [dpdk-stable] [PATCH] eal/headers: explicitly cast void * to type *

[Dmitry Kozlyuk]

On Wed, 13 Jan 2021 21:45:49 -0800, Tyler Retzlaff wrote:
> On Wed, Jan 13, 2021 at 08:52:55PM +0300, Dmitry Kozlyuk wrote:
> > On Mon, 11 Jan 2021 17:18:45 -0800, Tyler Retzlaff wrote:  
> > > Explicitly cast void * to type * so that eal headers may be compiled
> > > as C or C++.  
> > 
> > Topic should probably be "eal/windows".  
> 
> i'll submit a new rev that changes this, it's not really windows specific
> but i guess windows is the only people crazy enough to use c++.

1. Topic usually describe area of changes, rte_os.h is in windows directory.

2. It's a perfectly valid concern that public headers must be usable from C++.


> > > diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
> > > index bf715896a..c20be29b1 100644
> > > --- a/lib/librte_ethdev/rte_ethdev_pci.h
> > > +++ b/lib/librte_ethdev/rte_ethdev_pci.h
> > > @@ -47,7 +47,7 @@ rte_eth_copy_pci_info(struct rte_eth_dev *eth_dev,
> > >  
> > >  static inline int
> > >  eth_dev_pci_specific_init(struct rte_eth_dev *eth_dev, void *bus_device) {
> > > -	struct rte_pci_device *pci_dev = bus_device;
> > > +	struct rte_pci_device *pci_dev = (struct rte_pci_device *)bus_device;
> > >  
> > >  	if (!pci_dev)
> > >  		return -ENODEV;  
> > 
> > This is a private header, it's never exposed---why the change is needed (not
> > that I have a strong opinion, though)?  
> 
> interesting, i'll look into why/how it is being included and confirm. i suppose
> the question in the back of my mind is if it is private then why is the header
> being installed at all?

+ Bruce

If it's a public header then maybe it's missing a @file?

Re: [dpdk-stable] [PATCH] eal/headers: explicitly cast void * to type *

[Bruce Richardson]

On Thu, Jan 14, 2021 at 10:05:57AM +0300, Dmitry Kozlyuk wrote:
> On Wed, 13 Jan 2021 21:45:49 -0800, Tyler Retzlaff wrote:
> > On Wed, Jan 13, 2021 at 08:52:55PM +0300, Dmitry Kozlyuk wrote:
> > > On Mon, 11 Jan 2021 17:18:45 -0800, Tyler Retzlaff wrote:  
> > > > Explicitly cast void * to type * so that eal headers may be
> > > > compiled as C or C++.  
> > > 
> > > Topic should probably be "eal/windows".  
> > 
> > i'll submit a new rev that changes this, it's not really windows
> > specific but i guess windows is the only people crazy enough to use
> > c++.
> 
> 1. Topic usually describe area of changes, rte_os.h is in windows
> directory.
> 
> 2. It's a perfectly valid concern that public headers must be usable from
> C++.
> 
> 
> > > > diff --git a/lib/librte_ethdev/rte_ethdev_pci.h
> > > > b/lib/librte_ethdev/rte_ethdev_pci.h index bf715896a..c20be29b1
> > > > 100644 --- a/lib/librte_ethdev/rte_ethdev_pci.h +++
> > > > b/lib/librte_ethdev/rte_ethdev_pci.h @@ -47,7 +47,7 @@
> > > > rte_eth_copy_pci_info(struct rte_eth_dev *eth_dev,
> > > >  
> > > >  static inline int eth_dev_pci_specific_init(struct rte_eth_dev
> > > >  *eth_dev, void *bus_device) { -	struct rte_pci_device
> > > >  *pci_dev = bus_device; +	struct rte_pci_device *pci_dev =
> > > >  (struct rte_pci_device *)bus_device;
> > > >  
> > > >  	if (!pci_dev) return -ENODEV;  
> > > 
> > > This is a private header, it's never exposed---why the change is
> > > needed (not that I have a strong opinion, though)?  
> > 
> > interesting, i'll look into why/how it is being included and confirm. i
> > suppose the question in the back of my mind is if it is private then
> > why is the header being installed at all?
> 
> + Bruce
> 
> If it's a public header then maybe it's missing a @file?
>
My 2c on this in general...
 
The use of public vs private headers is not always clear, sadly, in DPDK,
for historical reasons.  With the make builds, libraries picked up headers
from other libraries via the "include" folder for all of DPDK, meaning that
if a particular header was internal only but used by multiple other libs,
it was placed in "include" for simplicity, rather than having each library
using it having to have separate "-I/path/to/header" cflags specified.
With the switch to meson, this common folder use is no longer be the case,
but because of the old way of doing things it may be that in the transition
some private headers were inadvertently kept as public (and possibly vice
versa, though that is more likely to be spotted by now).

/Bruce

Re: [dpdk-stable] [PATCH] eal/headers: explicitly cast void * to type *

[Dmitry Kozlyuk]

> > If it's a public header then maybe it's missing a @file?
> >  
> My 2c on this in general...
>  
> The use of public vs private headers is not always clear, sadly, in DPDK,
> for historical reasons.  With the make builds, libraries picked up headers
> from other libraries via the "include" folder for all of DPDK, meaning that
> if a particular header was internal only but used by multiple other libs,
> it was placed in "include" for simplicity, rather than having each library
> using it having to have separate "-I/path/to/header" cflags specified.
> With the switch to meson, this common folder use is no longer be the case,
> but because of the old way of doing things it may be that in the transition
> some private headers were inadvertently kept as public (and possibly vice
> versa, though that is more likely to be spotted by now).
> 
> /Bruce

Here is why rte_ethdev_pci.h should be considered private to DPDK:

* rte_eth_copy_pci_info - intended for device init, that is, driver job
* eth_dev_pci_specific_init - wrapper for the above
* rte_eth_dev_pci_allocate - @internal, deals with private data
* rte_eth_dev_pci_generic_probe - @internal, deals with private data
* rte_eth_dev_pci_generic_remove - @internal

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal/headers: explicitly cast void * to type *

[Thomas Monjalon]

14/01/2021 19:27, Dmitry Kozlyuk:
> > > If it's a public header then maybe it's missing a @file?
> > >  
> > My 2c on this in general...
> >  
> > The use of public vs private headers is not always clear, sadly, in DPDK,
> > for historical reasons.  With the make builds, libraries picked up headers
> > from other libraries via the "include" folder for all of DPDK, meaning that
> > if a particular header was internal only but used by multiple other libs,
> > it was placed in "include" for simplicity, rather than having each library
> > using it having to have separate "-I/path/to/header" cflags specified.
> > With the switch to meson, this common folder use is no longer be the case,
> > but because of the old way of doing things it may be that in the transition
> > some private headers were inadvertently kept as public (and possibly vice
> > versa, though that is more likely to be spotted by now).
> > 
> > /Bruce
> 
> Here is why rte_ethdev_pci.h should be considered private to DPDK:
> 
> * rte_eth_copy_pci_info - intended for device init, that is, driver job
> * eth_dev_pci_specific_init - wrapper for the above
> * rte_eth_dev_pci_allocate - @internal, deals with private data
> * rte_eth_dev_pci_generic_probe - @internal, deals with private data
> * rte_eth_dev_pci_generic_remove - @internal

Yes rte_ethdev_pci.h is a helper for ethdev drivers,
it is DPDK internal.

Re: [dpdk-stable] [PATCH] eal/headers: explicitly cast void * to type *

[Tyler Retzlaff]

On Thu, Jan 14, 2021 at 10:55:54AM +0000, Bruce Richardson wrote:
> > > > This is a private header, it's never exposed---why the change is
> > > > needed (not that I have a strong opinion, though)?  
> > > 
> > > interesting, i'll look into why/how it is being included and confirm. i
> > > suppose the question in the back of my mind is if it is private then
> > > why is the header being installed at all?

okay, i now understand how we ended up compiling this header directly. long
story short we just had it #include directly somewhere.

as you have noted it is a private header. a quick examination of the other
installed headers shows that it is not included by any of them.

i will update the patch to remove the cast from rte_ethdev_pci.h since it
is not necessary.

would you also like a patch submitted that stops installing the header. the
change will be breaking if any other consumers have made the same mistake as
we did. i'm not sure what dpdk's stance is on pulling headers back out of
public space.

thanks

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal/headers: explicitly cast void * to type *

[Thomas Monjalon]

15/01/2021 20:21, Tyler Retzlaff:
> would you also like a patch submitted that stops installing the header. the
> change will be breaking if any other consumers have made the same mistake as
> we did. i'm not sure what dpdk's stance is on pulling headers back out of
> public space.

That's a good question.
If it is described enough that it is not part of the API,
I think we can stop installing the header.
Anyway, our only commitment is on ABI compatibility, so it should be OK.

[dpdk-stable] [PATCH v2] eal/windows: explicitly cast void * to type *

[Tyler Retzlaff]

Explicitly cast void * to type * so that eal headers may be compiled
as C or C++.

Fixes: e8428a9d89f1 ("eal/windows: add some basic functions and macros")
Cc: stable@dpdk.org

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/librte_eal/windows/include/rte_os.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/windows/include/rte_os.h b/lib/librte_eal/windows/include/rte_os.h
index ea3fe60e5..7ef38ff06 100644
--- a/lib/librte_eal/windows/include/rte_os.h
+++ b/lib/librte_eal/windows/include/rte_os.h
@@ -86,7 +86,7 @@ asprintf(char **buffer, const char *format, ...)
 		return -1;
 	size++;
 
-	*buffer = malloc(size);
+	*buffer = (char *)malloc(size);
 	if (*buffer == NULL)
 		return -1;
 
-- 
2.29.0.vfs.0.0

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] eal/windows: explicitly cast void * to type *

[Thomas Monjalon]

15/01/2021 20:38, Tyler Retzlaff:
> Explicitly cast void * to type * so that eal headers may be compiled
> as C or C++.
> 
> Fixes: e8428a9d89f1 ("eal/windows: add some basic functions and macros")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Applied with title "eal/windows: fix C++ compatibility", thanks.

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] eal/windows: explicitly cast void * to type *

[Stephen Hemminger]

On Fri, 15 Jan 2021 11:38:21 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> Explicitly cast void * to type * so that eal headers may be compiled
> as C or C++.
> 
> Fixes: e8428a9d89f1 ("eal/windows: add some basic functions and macros")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  lib/librte_eal/windows/include/rte_os.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/windows/include/rte_os.h b/lib/librte_eal/windows/include/rte_os.h
> index ea3fe60e5..7ef38ff06 100644
> --- a/lib/librte_eal/windows/include/rte_os.h
> +++ b/lib/librte_eal/windows/include/rte_os.h
> @@ -86,7 +86,7 @@ asprintf(char **buffer, const char *format, ...)
>  		return -1;
>  	size++;
>  
> -	*buffer = malloc(size);
> +	*buffer = (char *)malloc(size);
>  	if (*buffer == NULL)
>  		return -1;
>  

Why is the compiler enforcing C++ rules on code that is inside "extern C {"?

Bigger question is why is this code inlined? It is not critical path
and should be a function.

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] eal/windows: explicitly cast void * to type *

[Dmitry Kozlyuk]

On Sun, 17 Jan 2021 10:10:39 -0800, Stephen Hemminger wrote: 
> > -	*buffer = malloc(size);
> > +	*buffer = (char *)malloc(size);
> >  	if (*buffer == NULL)
> >  		return -1;
> >    
> 
> Why is the compiler enforcing C++ rules on code that is inside "extern C {"?

Code inside extern "C" is not compiled as C; directive only affects linkage.

> Bigger question is why is this code inlined? It is not critical path
> and should be a function.

Absolutely.
There's more: windows/rte_os.h should not expose POSIX symbols at all, I'm
working on a patchset to clean it up and un-inline this code.