Re: [PATCH v14 04/11] app/test: skip interrupt tests on Windows

[Thomas Monjalon <thomas@monjalon.net>]

The proposal below is now merged.
Please Jie, use it in a v15 of this series.


10/12/2021 10:30, Bruce Richardson:
> On Fri, Dec 10, 2021 at 12:23:59PM +0300, Dmitry Kozlyuk wrote:
> > 2021-12-09 16:39 (UTC+0000), Bruce Richardson:
> > > On Thu, Dec 09, 2021 at 04:17:08PM +0000, Bruce Richardson wrote:
> > > [...]
> > > > I'm wondering if a reasonable compromise solution might be to have the
> > > > build system expose a usable RTE_EXEC_ENV symbol that can be used in C-code
> > > > if statements rather than just in ifdefs. That would allow us to easily add
> > > > e.g.
> > > > 
> > > > if (RTE_EXEC_ENV == rte_env_linux)
> > > >     return TEST_SKIPPED;
> > > > 
> > > > into each test function needing it. Two lines of C-code is a lot easier to
> > > > add and manage than #ifdefs covering the whole file, or alternative lists
> > > > in meson.
> > > >   
> > > Quick patch to allow C-code comparisons:
> > > 
> > > diff --git a/lib/eal/meson.build b/lib/eal/meson.build
> > > index 1722924f67..b5b9fa14b4 100644
> > > --- a/lib/eal/meson.build
> > > +++ b/lib/eal/meson.build
> > > @@ -10,6 +10,12 @@ if not is_windows
> > >      subdir('unix')
> > >  endif
> > >  
> > > +exec_envs = {'freebsd': 0, 'linux': 1, 'windows': 2}
> > > +foreach env, id:exec_envs
> > > +    dpdk_conf.set('RTE_ENV_' + env.to_upper(), id)
> > > +endforeach
> > > +dpdk_conf.set('RTE_EXEC_ENV', exec_envs[exec_env])
> > > +
> > >  dpdk_conf.set('RTE_EXEC_ENV_' + exec_env.to_upper(), 1)
> > >  subdir(exec_env)
> > > 
> > > A slightly simpler patch would just expose the environment as a string as
> > > e.g. "linux", but I think numeric ids just make the code better rather than
> > > having string comparisons. Alternatively, this could also be done via
> > > C-code with ifdefs in EAL, but as it stands this meson change allows:
> > > 
> > >   if (RTE_EXEC_ENV == RTE_ENV_WINDOWS)
> > >      ...
> > > 
> > > or:
> > > 
> > >   switch (RTE_EXEC_ENV) {
> > >     case RTE_ENV_LINUX: ... ; break;
> > >     case RTE_ENV_FREEBSD: ... ; break;
> > >     case RTE_ENV_WINDOWS: ... ; break;
> > >   }
> > > 
> > > Thoughts?
> > 
> > I like this.
> > Even outside of tests more code can be made to compile on all platforms
> > (e.g. ixgbe_wait_for_link_up).
> > Alternative naming: RTE_EXEC_ENV_IS_* (similar to RTE_CC_IS_*),
> > which does not allow switch statements, but shortens most practical cases.
> 
> Sure. I wonder if it is worthwhile implementing both, since it's not a
> large amount of code.
> 
> > Will Coverity understand that if a condition is always false,
> > variables beneath still may be used on another platform?
> 
> That I don't know, unfortunately. Perhaps some coverity experts can weigh
> in.