On Fri, Jun 17, 2022 at 4:43 AM fengchengwen <[email protected]> wrote: > > Hi David, thanks for your review. > > On 2022/6/15 22:00, David Marchand wrote: > > On Tue, Jun 14, 2022 at 8:06 AM Chengwen Feng <[email protected]> > > wrote: > >> > >> There are a bug[1] when exit application while enable tracing, this > >> bug has not been discovered for a long time, to quickly detect such > >> bugs, this patch was introduced. > >> > >> This patch adds a testcase with trace enabling, it also depends on > >> patch[2] because it has a long file-prefix. > >> > >> [1] eal: fix segment fault when exit trace > >> [2] eal: fix trace init fail with long file-prefix > > > > This commitlog feels more like a cover letter. > > > > Please describe what the impact of the patch is, like mention that the > > trace_autotest unit test is being called twice, once with traces > > disabled, and once with traces enabled. > > And that the traces file is being written in a directory part of the > > build directory. > > > > > >> > >> Signed-off-by: Chengwen Feng <[email protected]> > >> --- > >> app/test/meson.build | 11 +++++++++++ > >> 1 file changed, 11 insertions(+) > >> > >> diff --git a/app/test/meson.build b/app/test/meson.build > >> index 7fe261cae8..eb37aa632a 100644 > >> --- a/app/test/meson.build > >> +++ b/app/test/meson.build > >> @@ -509,6 +509,17 @@ foreach arg : fast_tests > >> is_parallel : false, > >> suite : 'fast-tests') > >> endif > >> + > >> + if run_test and arg[0] == 'trace_autotest' and (not is_windows) > > > > Calling this test on Windows should not be an issue. > > Is it not the case? > > I notice that eal_parse_common_option still keep windows macro for trace > parameter parse: > > #ifndef RTE_EXEC_ENV_WINDOWS > case OPT_TRACE_NUM: { > ... > case OPT_TRACE_MODE_NUM: { > if (eal_trace_mode_args_save(optarg) < 0) { > RTE_LOG(ERR, EAL, "invalid parameters for --" > OPT_TRACE_MODE "\n"); > return -1; > } > break; > } > #endif /* !RTE_EXEC_ENV_WINDOWS */ > > Maybe trace already support in windows, but I havn't the windows enviorment > to verify it. > > So I think we should keep '(not is_windows)' currently.
Indeed. -- David Marchand
