eal/headers: explicitly cast void * to type *

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

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/librte_eal/windows/include/rte_os.h | 2 +-
 lib/librte_ethdev/rte_ethdev_pci.h      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
  

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)?
  

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?
  

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?
  

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
  

> > 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
  

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.
  

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
  

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.