patches for DPDK stable branches
 help / color / mirror / Atom feed
* Re: [dpdk-stable] [PATCH] eal/headers: explicitly cast void * to type *
       [not found] <1610414325-9104-1-git-send-email-roretzla@linux.microsoft.com>
@ 2021-01-13 17:52 ` Dmitry Kozlyuk
  2021-01-14  5:45   ` Tyler Retzlaff
  2021-01-15 19:38 ` [dpdk-stable] [PATCH v2] eal/windows: " Tyler Retzlaff
  1 sibling, 1 reply; 12+ messages in thread
From: Dmitry Kozlyuk @ 2021-01-13 17:52 UTC (permalink / raw)
  To: Tyler Retzlaff; +Cc: dev, navasile, stable

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [PATCH] eal/headers: explicitly cast void * to type *
  2021-01-13 17:52 ` [dpdk-stable] [PATCH] eal/headers: explicitly cast void * to type * Dmitry Kozlyuk
@ 2021-01-14  5:45   ` Tyler Retzlaff
  2021-01-14  7:05     ` Dmitry Kozlyuk
  0 siblings, 1 reply; 12+ messages in thread
From: Tyler Retzlaff @ 2021-01-14  5:45 UTC (permalink / raw)
  To: Dmitry Kozlyuk; +Cc: dev, navasile, stable

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?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [PATCH] eal/headers: explicitly cast void * to type *
  2021-01-14  5:45   ` Tyler Retzlaff
@ 2021-01-14  7:05     ` Dmitry Kozlyuk
  2021-01-14 10:55       ` Bruce Richardson
  0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Kozlyuk @ 2021-01-14  7:05 UTC (permalink / raw)
  To: Tyler Retzlaff; +Cc: dev, navasile, stable, Bruce Richardson

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?


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [PATCH] eal/headers: explicitly cast void * to type *
  2021-01-14  7:05     ` Dmitry Kozlyuk
@ 2021-01-14 10:55       ` Bruce Richardson
  2021-01-14 18:27         ` Dmitry Kozlyuk
  2021-01-15 19:21         ` [dpdk-stable] " Tyler Retzlaff
  0 siblings, 2 replies; 12+ messages in thread
From: Bruce Richardson @ 2021-01-14 10:55 UTC (permalink / raw)
  To: Dmitry Kozlyuk; +Cc: Tyler Retzlaff, dev, navasile, stable

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [PATCH] eal/headers: explicitly cast void * to type *
  2021-01-14 10:55       ` Bruce Richardson
@ 2021-01-14 18:27         ` Dmitry Kozlyuk
  2021-01-14 18:49           ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
  2021-01-15 19:21         ` [dpdk-stable] " Tyler Retzlaff
  1 sibling, 1 reply; 12+ messages in thread
From: Dmitry Kozlyuk @ 2021-01-14 18:27 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: Tyler Retzlaff, dev, navasile, stable

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] eal/headers: explicitly cast void * to type *
  2021-01-14 18:27         ` Dmitry Kozlyuk
@ 2021-01-14 18:49           ` Thomas Monjalon
  0 siblings, 0 replies; 12+ messages in thread
From: Thomas Monjalon @ 2021-01-14 18:49 UTC (permalink / raw)
  To: Bruce Richardson, Tyler Retzlaff, Dmitry Kozlyuk; +Cc: dev, navasile, stable

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.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [PATCH] eal/headers: explicitly cast void * to type *
  2021-01-14 10:55       ` Bruce Richardson
  2021-01-14 18:27         ` Dmitry Kozlyuk
@ 2021-01-15 19:21         ` Tyler Retzlaff
  2021-01-17 17:13           ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
  1 sibling, 1 reply; 12+ messages in thread
From: Tyler Retzlaff @ 2021-01-15 19:21 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: Dmitry Kozlyuk, dev, navasile, stable

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-stable] [PATCH v2] eal/windows: explicitly cast void * to type *
       [not found] <1610414325-9104-1-git-send-email-roretzla@linux.microsoft.com>
  2021-01-13 17:52 ` [dpdk-stable] [PATCH] eal/headers: explicitly cast void * to type * Dmitry Kozlyuk
@ 2021-01-15 19:38 ` Tyler Retzlaff
  2021-01-17 17:19   ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
  2021-01-17 18:10   ` Stephen Hemminger
  1 sibling, 2 replies; 12+ messages in thread
From: Tyler Retzlaff @ 2021-01-15 19:38 UTC (permalink / raw)
  To: dev; +Cc: stable, dmitry.kozliuk

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] eal/headers: explicitly cast void * to type *
  2021-01-15 19:21         ` [dpdk-stable] " Tyler Retzlaff
@ 2021-01-17 17:13           ` Thomas Monjalon
  0 siblings, 0 replies; 12+ messages in thread
From: Thomas Monjalon @ 2021-01-17 17:13 UTC (permalink / raw)
  To: Tyler Retzlaff; +Cc: Bruce Richardson, Dmitry Kozlyuk, dev, navasile, stable

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.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] eal/windows: explicitly cast void * to type *
  2021-01-15 19:38 ` [dpdk-stable] [PATCH v2] eal/windows: " Tyler Retzlaff
@ 2021-01-17 17:19   ` Thomas Monjalon
  2021-01-17 18:10   ` Stephen Hemminger
  1 sibling, 0 replies; 12+ messages in thread
From: Thomas Monjalon @ 2021-01-17 17:19 UTC (permalink / raw)
  To: Tyler Retzlaff; +Cc: dev, stable, dmitry.kozliuk

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.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] eal/windows: explicitly cast void * to type *
  2021-01-15 19:38 ` [dpdk-stable] [PATCH v2] eal/windows: " Tyler Retzlaff
  2021-01-17 17:19   ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
@ 2021-01-17 18:10   ` Stephen Hemminger
  2021-01-17 19:51     ` Dmitry Kozlyuk
  1 sibling, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2021-01-17 18:10 UTC (permalink / raw)
  To: Tyler Retzlaff; +Cc: dev, stable, dmitry.kozliuk

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] eal/windows: explicitly cast void * to type *
  2021-01-17 18:10   ` Stephen Hemminger
@ 2021-01-17 19:51     ` Dmitry Kozlyuk
  0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Kozlyuk @ 2021-01-17 19:51 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Tyler Retzlaff, dev, stable

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-01-17 19:51 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1610414325-9104-1-git-send-email-roretzla@linux.microsoft.com>
2021-01-13 17:52 ` [dpdk-stable] [PATCH] eal/headers: explicitly cast void * to type * Dmitry Kozlyuk
2021-01-14  5:45   ` Tyler Retzlaff
2021-01-14  7:05     ` Dmitry Kozlyuk
2021-01-14 10:55       ` Bruce Richardson
2021-01-14 18:27         ` Dmitry Kozlyuk
2021-01-14 18:49           ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
2021-01-15 19:21         ` [dpdk-stable] " Tyler Retzlaff
2021-01-17 17:13           ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
2021-01-15 19:38 ` [dpdk-stable] [PATCH v2] eal/windows: " Tyler Retzlaff
2021-01-17 17:19   ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
2021-01-17 18:10   ` Stephen Hemminger
2021-01-17 19:51     ` Dmitry Kozlyuk

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).