Re: [dpdk-dev] [PATCH 6/7] cmdline: support Windows

From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
To: Tal Shnaiderman <talshn@mellanox.com>
Cc: Ranjit Menon <ranjit.menon@intel.com>,
	Fady Bader <fady@mellanox.com>, "dev@dpdk.org" <dev@dpdk.org>,
	Dmitry Malloy <dmitrym@microsoft.com>,
	Narcisa Ana Maria Vasile <Narcisa.Vasile@microsoft.com>,
	Thomas Monjalon <thomas@monjalon.net>,
	Olivier Matz <olivier.matz@6wind.com>
Subject: Re: [dpdk-dev] [PATCH 6/7] cmdline: support Windows
Date: Wed, 8 Jul 2020 04:09:59 +0300	[thread overview]
Message-ID: <20200708040959.0fdbfbd2@sovereign> (raw)
In-Reply-To: <20200630025620.241e6d54@sovereign>

On Tue, 30 Jun 2020 02:56:20 +0300, Dmitry Kozlyuk wrote:
> On Mon, 29 Jun 2020 08:12:51 +0000, Tal Shnaiderman wrote:
> > > From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > > Subject: Re: [dpdk-dev] [PATCH 6/7] cmdline: support Windows
> > > 
> > > On Sun, 28 Jun 2020 23:23:11 -0700, Ranjit Menon wrote:    
[snip]
> > > > The issue is that UINT8, UINT16, INT32, INT64 etc. are reserved types
> > > > in Windows headers for integer types. We found that it is easier to
> > > > change the enum in cmdline_parse_num.h than try to play with the
> > > > include order of headers. AFAIK, the enums were only used to determine
> > > > the type in a series of switch() statements in librte_cmdline, so we
> > > > simply renamed the enums. Not sure, if that will be acceptable here.    
> > > 
> > > +1 for renaming enum values. It's not a problem of librte_cmdline itself
> > > +but a
> > > problem of its consumption on Windows, however renaming enum values
> > > doesn't break ABI and winn make librte_cmdline API "namespaced".
> > > 
[snip]
> > 
> > test_pmd redefine BOOLEAN and PATTERN in the index enum, I'm not sure how many more conflicts we will face because of this huge include.
> >
> > Also, DPDK applications will inherit it unknowingly, not sure if this is common for windows libraries.  
> 
> I never hit these particular conflicts, but you're right that there will be
> more, e.g. I remember particularly nasty clashes in failsafe PMD, unrelated
> to cmdline token names.

Still, I'd go for renaming, with or without additional steps to hide
<windows.h>. Although I wouldn't include it in this series: renaming will
touch numerous places and require much more reviewers.

> We could take the same approach as with networking headers: copy required
> declarations instead of including them from SDK. Here's a list of what
> pthread.h uses:

While this will resolve the issue for DPDK code, applications using DPDK
headers can easily hit it by including <windows.h> on their own. On the other
hand, they can always split translation units and I don't know how practical
it is to use system and DPDK networking headers at the same time.

-- 
Dmitry Kozlyuk