diff mbox series

[v8,03/11] app/test: replace POSIX specific code

Message ID 1635216361-23641-4-git-send-email-jizh@linux.microsoft.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series app/test: enable subset of tests on Windows |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Jie Zhou Oct. 26, 2021, 2:45 a.m. UTC 
  - Include rte_os_shim.h
- Replace sleep and usleep with rte_delay_us_sleep
- #ifndef RTE_EXEC_ENV_WINDOWS for POSIX code only

Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
---
 app/test/packet_burst_generator.c | 1 +
 app/test/process.h                | 4 +++-
 app/test/test.c                   | 4 ++++
 app/test/test_byteorder.c         | 2 +-
 app/test/test_cmdline.c           | 2 ++
 app/test/test_crc.c               | 1 -
 app/test/test_mp_secondary.c      | 2 ++
 app/test/test_pmd_perf.c          | 6 +++++-
 app/test/test_ring_stress.c       | 3 ++-
 app/test/test_ring_stress_impl.h  | 2 +-
 app/test/test_telemetry_data.c    | 2 ++
 11 files changed, 23 insertions(+), 6 deletions(-)

Comments

Dmitry Kozlyuk Nov. 23, 2021, 10:02 p.m. UTC | #1 
2021-10-25 19:45 (UTC-0700), Jie Zhou:
> - Include rte_os_shim.h
> - Replace sleep and usleep with rte_delay_us_sleep
> - #ifndef RTE_EXEC_ENV_WINDOWS for POSIX code only
> 
> Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> ---

This patch can be combined with the previous one:
they serve the same purpose---to remove Unix-specific code.

Please try to summarize in the commit message
which parts of the tests suites are excluded, e.g. multi-process.
It is more useful then stating what was changed in the code.

[...]
> diff --git a/app/test/test_cmdline.c b/app/test/test_cmdline.c
> index 115bee966d..9a76bd299f 100644
> --- a/app/test/test_cmdline.c
> +++ b/app/test/test_cmdline.c
> @@ -31,6 +31,7 @@ test_cmdline(void)
>  		return -1;
>  	if (test_parse_num_invalid_param() < 0)
>  		return -1;
> +#ifndef RTE_EXEC_ENV_WINDOWS
>  	printf("Testing parsing IP addresses...\n");
>  	if (test_parse_ipaddr_valid() < 0)
>  		return -1;
> @@ -38,6 +39,7 @@ test_cmdline(void)
>  		return -1;
>  	if (test_parse_ipaddr_invalid_param() < 0)
>  		return -1;
> +#endif
>  	printf("Testing parsing strings...\n");
>  	if (test_parse_string_valid() < 0)
>  		return -1;

What's wrong with parsing IP addresses on Windows?

[...]
> diff --git a/app/test/test_mp_secondary.c b/app/test/test_mp_secondary.c
> index 5b6f05dbb1..da035348bd 100644
> --- a/app/test/test_mp_secondary.c
> +++ b/app/test/test_mp_secondary.c
> @@ -14,7 +14,9 @@
>  #include <errno.h>
>  #include <string.h>
>  #include <unistd.h>
> +#ifndef RTE_EXEC_ENV_WINDOWS
>  #include <sys/wait.h>
> +#endif
>  #include <libgen.h>
>  #include <dirent.h>
>  #include <limits.h>

<libgen.h> is absent on Windows for sure, but you don't exclude it.
Does this file even need modification?
It's not going to be compiled for Windows.

[...]
> diff --git a/app/test/test_ring_stress.c b/app/test/test_ring_stress.c
> index 1af45e0fc8..ce3535c6b2 100644
> --- a/app/test/test_ring_stress.c
> +++ b/app/test/test_ring_stress.c
> @@ -43,9 +43,10 @@ test_ring_stress(void)
>  	n += test_ring_rts_stress.nb_case;
>  	k += run_test(&test_ring_rts_stress);
>  
> +#ifndef RTE_EXEC_ENV_WINDOWS
>  	n += test_ring_hts_stress.nb_case;
>  	k += run_test(&test_ring_hts_stress);
> -
> +#endif

Can you please elaborate what is the issue with this case?
It is also one of the details you usually want to put
into the commit message.
Jie Zhou Dec. 1, 2021, 1:05 a.m. UTC | #2 
On Wed, Nov 24, 2021 at 01:02:06AM +0300, Dmitry Kozlyuk wrote:
> 2021-10-25 19:45 (UTC-0700), Jie Zhou:
> > - Include rte_os_shim.h
> > - Replace sleep and usleep with rte_delay_us_sleep
> > - #ifndef RTE_EXEC_ENV_WINDOWS for POSIX code only
> > 
> > Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> > ---
> 
> This patch can be combined with the previous one:
> they serve the same purpose---to remove Unix-specific code.
> 
> Please try to summarize in the commit message
> which parts of the tests suites are excluded, e.g. multi-process.
> It is more useful then stating what was changed in the code.
>
Will combine and revise the message.
 
> [...]
> > diff --git a/app/test/test_cmdline.c b/app/test/test_cmdline.c
> > index 115bee966d..9a76bd299f 100644
> > --- a/app/test/test_cmdline.c
> > +++ b/app/test/test_cmdline.c
> > @@ -31,6 +31,7 @@ test_cmdline(void)
> >  		return -1;
> >  	if (test_parse_num_invalid_param() < 0)
> >  		return -1;
> > +#ifndef RTE_EXEC_ENV_WINDOWS
> >  	printf("Testing parsing IP addresses...\n");
> >  	if (test_parse_ipaddr_valid() < 0)
> >  		return -1;
> > @@ -38,6 +39,7 @@ test_cmdline(void)
> >  		return -1;
> >  	if (test_parse_ipaddr_invalid_param() < 0)
> >  		return -1;
> > +#endif
> >  	printf("Testing parsing strings...\n");
> >  	if (test_parse_string_valid() < 0)
> >  		return -1;
> 
> What's wrong with parsing IP addresses on Windows?
> 
test_cmdline_ipaddr.c uses linux netinet/in.h specific u6_addr. Skip these 3 cases for now and prefer a separate patch to make it work on Windows. Or maybe there is already DPDK support on this which I am not aware of? Thanks.

> [...]
> > diff --git a/app/test/test_mp_secondary.c b/app/test/test_mp_secondary.c
> > index 5b6f05dbb1..da035348bd 100644
> > --- a/app/test/test_mp_secondary.c
> > +++ b/app/test/test_mp_secondary.c
> > @@ -14,7 +14,9 @@
> >  #include <errno.h>
> >  #include <string.h>
> >  #include <unistd.h>
> > +#ifndef RTE_EXEC_ENV_WINDOWS
> >  #include <sys/wait.h>
> > +#endif
> >  #include <libgen.h>
> >  #include <dirent.h>
> >  #include <limits.h>
> 
> <libgen.h> is absent on Windows for sure, but you don't exclude it.
> Does this file even need modification?
> It's not going to be compiled for Windows.
> 
This is replaced with the test stub in the patch#11 (of V8) to make it compile on Windows. Sorry for the confusion. Will make sure remove this unnecessary part from V9.

> [...]
> > diff --git a/app/test/test_ring_stress.c b/app/test/test_ring_stress.c
> > index 1af45e0fc8..ce3535c6b2 100644
> > --- a/app/test/test_ring_stress.c
> > +++ b/app/test/test_ring_stress.c
> > @@ -43,9 +43,10 @@ test_ring_stress(void)
> >  	n += test_ring_rts_stress.nb_case;
> >  	k += run_test(&test_ring_rts_stress);
> >  
> > +#ifndef RTE_EXEC_ENV_WINDOWS
> >  	n += test_ring_hts_stress.nb_case;
> >  	k += run_test(&test_ring_hts_stress);
> > -
> > +#endif
> 
> Can you please elaborate what is the issue with this case?
> It is also one of the details you usually want to put
> into the commit message.

Cannot remember what caused this case being skipped in the first place, but it can work now. So removed the ifndef. Thanks.
Dmitry Kozlyuk Dec. 1, 2021, 7:19 a.m. UTC | #3 
2021-11-30 17:05 (UTC-0800), Jie Zhou:
> On Wed, Nov 24, 2021 at 01:02:06AM +0300, Dmitry Kozlyuk wrote:
[...]
> > [...]  
> > > diff --git a/app/test/test_cmdline.c b/app/test/test_cmdline.c
> > > index 115bee966d..9a76bd299f 100644
> > > --- a/app/test/test_cmdline.c
> > > +++ b/app/test/test_cmdline.c
> > > @@ -31,6 +31,7 @@ test_cmdline(void)
> > >  		return -1;
> > >  	if (test_parse_num_invalid_param() < 0)
> > >  		return -1;
> > > +#ifndef RTE_EXEC_ENV_WINDOWS
> > >  	printf("Testing parsing IP addresses...\n");
> > >  	if (test_parse_ipaddr_valid() < 0)
> > >  		return -1;
> > > @@ -38,6 +39,7 @@ test_cmdline(void)
> > >  		return -1;
> > >  	if (test_parse_ipaddr_invalid_param() < 0)
> > >  		return -1;
> > > +#endif
> > >  	printf("Testing parsing strings...\n");
> > >  	if (test_parse_string_valid() < 0)
> > >  		return -1;  
> > 
> > What's wrong with parsing IP addresses on Windows?
> >   
> test_cmdline_ipaddr.c uses linux netinet/in.h specific u6_addr. Skip these
> 3 cases for now and prefer a separate patch to make it work on Windows. Or
> maybe there is already DPDK support on this which I am not aware of? Thanks.

Understood, only please explain this in the commit log.
diff mbox series

Patch

diff --git a/app/test/packet_burst_generator.c b/app/test/packet_burst_generator.c
index 8ac24577ba..6b42b9b83b 100644
--- a/app/test/packet_burst_generator.c
+++ b/app/test/packet_burst_generator.c
@@ -5,6 +5,7 @@ 
 #include <rte_byteorder.h>
 #include <rte_mbuf.h>
 #include <rte_ip.h>
+#include <rte_os_shim.h>
 
 #include "packet_burst_generator.h"
 
diff --git a/app/test/process.h b/app/test/process.h
index 5b10cf64df..1f073b9c5c 100644
--- a/app/test/process.h
+++ b/app/test/process.h
@@ -7,12 +7,14 @@ 
 
 #include <errno.h>  /* errno */
 #include <limits.h> /* PATH_MAX */
+#ifndef RTE_EXEC_ENV_WINDOWS
 #include <libgen.h> /* basename et al */
+#include <sys/wait.h>
+#endif
 #include <stdlib.h> /* NULL */
 #include <string.h> /* strerror */
 #include <unistd.h> /* readlink */
 #include <dirent.h>
-#include <sys/wait.h>
 
 #include <rte_string_fns.h> /* strlcpy */
 
diff --git a/app/test/test.c b/app/test/test.c
index 8b698656bf..e69cae3eea 100644
--- a/app/test/test.c
+++ b/app/test/test.c
@@ -62,7 +62,9 @@  do_recursive_call(void)
 		const char *env_var;
 		int (*action_fn)(void);
 	} actions[] =  {
+#ifndef RTE_EXEC_ENV_WINDOWS
 			{ "run_secondary_instances", test_mp_secondary },
+#endif
 #ifdef RTE_LIB_PDUMP
 #ifdef RTE_NET_RING
 			{ "run_pdump_server_tests", test_pdump },
@@ -81,7 +83,9 @@  do_recursive_call(void)
 			{ "test_file_prefix", no_action },
 			{ "test_no_huge_flag", no_action },
 #ifdef RTE_LIB_TIMER
+#ifndef RTE_EXEC_ENV_WINDOWS
 			{ "timer_secondary_spawn_wait", test_timer_secondary },
+#endif
 #endif
 	};
 
diff --git a/app/test/test_byteorder.c b/app/test/test_byteorder.c
index 03c08d9abf..de14ed539e 100644
--- a/app/test/test_byteorder.c
+++ b/app/test/test_byteorder.c
@@ -46,7 +46,7 @@  test_byteorder(void)
 		return -1;
 
 	res_u16 = rte_bswap16(0x1337);
-	printf("const %"PRIx16" -> %"PRIx16"\n", 0x1337, res_u16);
+	printf("const %"PRIx16" -> %"PRIx16"\n", (uint16_t)0x1337, res_u16);
 	if (res_u16 != 0x3713)
 		return -1;
 
diff --git a/app/test/test_cmdline.c b/app/test/test_cmdline.c
index 115bee966d..9a76bd299f 100644
--- a/app/test/test_cmdline.c
+++ b/app/test/test_cmdline.c
@@ -31,6 +31,7 @@  test_cmdline(void)
 		return -1;
 	if (test_parse_num_invalid_param() < 0)
 		return -1;
+#ifndef RTE_EXEC_ENV_WINDOWS
 	printf("Testing parsing IP addresses...\n");
 	if (test_parse_ipaddr_valid() < 0)
 		return -1;
@@ -38,6 +39,7 @@  test_cmdline(void)
 		return -1;
 	if (test_parse_ipaddr_invalid_param() < 0)
 		return -1;
+#endif
 	printf("Testing parsing strings...\n");
 	if (test_parse_string_valid() < 0)
 		return -1;
diff --git a/app/test/test_crc.c b/app/test/test_crc.c
index bf1d344359..0ed080e482 100644
--- a/app/test/test_crc.c
+++ b/app/test/test_crc.c
@@ -14,7 +14,6 @@ 
 #define CRC32_VEC_LEN2     348
 #define CRC16_VEC_LEN1     12
 #define CRC16_VEC_LEN2     2
-#define LINE_LEN           75
 
 /* CRC test vector */
 static const uint8_t crc_vec[CRC_VEC_LEN] = {
diff --git a/app/test/test_mp_secondary.c b/app/test/test_mp_secondary.c
index 5b6f05dbb1..da035348bd 100644
--- a/app/test/test_mp_secondary.c
+++ b/app/test/test_mp_secondary.c
@@ -14,7 +14,9 @@ 
 #include <errno.h>
 #include <string.h>
 #include <unistd.h>
+#ifndef RTE_EXEC_ENV_WINDOWS
 #include <sys/wait.h>
+#endif
 #include <libgen.h>
 #include <dirent.h>
 #include <limits.h>
diff --git a/app/test/test_pmd_perf.c b/app/test/test_pmd_perf.c
index 1df86ce080..b5e5a93a52 100644
--- a/app/test/test_pmd_perf.c
+++ b/app/test/test_pmd_perf.c
@@ -297,6 +297,7 @@  reset_count(void)
 	idle = 0;
 }
 
+#ifndef RTE_EXEC_ENV_WINDOWS
 static void
 stats_display(uint16_t port_id)
 {
@@ -326,6 +327,7 @@  signal_handler(int signum)
 	if (signum == SIGUSR2)
 		stats_display(0);
 }
+#endif
 
 struct rte_mbuf **tx_burst;
 
@@ -637,7 +639,7 @@  exec_burst(uint32_t flags, int lcore)
 		i = (i >= conf->nb_ports - 1) ? 0 : (i + 1);
 	}
 
-	sleep(5);
+	rte_delay_us(5 * US_PER_S);
 
 	/* only when polling second  */
 	if (flags == SC_BURST_XMIT_FIRST)
@@ -668,8 +670,10 @@  test_pmd_perf(void)
 
 	printf("Start PMD RXTX cycles cost test.\n");
 
+#ifndef RTE_EXEC_ENV_WINDOWS
 	signal(SIGUSR1, signal_handler);
 	signal(SIGUSR2, signal_handler);
+#endif
 
 	nb_ports = rte_eth_dev_count_avail();
 	if (nb_ports < NB_ETHPORTS_USED) {
diff --git a/app/test/test_ring_stress.c b/app/test/test_ring_stress.c
index 1af45e0fc8..ce3535c6b2 100644
--- a/app/test/test_ring_stress.c
+++ b/app/test/test_ring_stress.c
@@ -43,9 +43,10 @@  test_ring_stress(void)
 	n += test_ring_rts_stress.nb_case;
 	k += run_test(&test_ring_rts_stress);
 
+#ifndef RTE_EXEC_ENV_WINDOWS
 	n += test_ring_hts_stress.nb_case;
 	k += run_test(&test_ring_hts_stress);
-
+#endif
 	n += test_ring_mt_peek_stress.nb_case;
 	k += run_test(&test_ring_mt_peek_stress);
 
diff --git a/app/test/test_ring_stress_impl.h b/app/test/test_ring_stress_impl.h
index 2825a9dce6..62f046a91a 100644
--- a/app/test/test_ring_stress_impl.h
+++ b/app/test/test_ring_stress_impl.h
@@ -360,7 +360,7 @@  test_mt1(int (*test)(void *))
 	/* signal worker to start test */
 	__atomic_store_n(&wrk_cmd, WRK_CMD_RUN, __ATOMIC_RELEASE);
 
-	usleep(run_time * US_PER_S);
+	rte_delay_us(run_time * US_PER_S);
 
 	/* signal worker to start test */
 	__atomic_store_n(&wrk_cmd, WRK_CMD_STOP, __ATOMIC_RELEASE);
diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
index 18b93db8ef..73eee293a1 100644
--- a/app/test/test_telemetry_data.c
+++ b/app/test/test_telemetry_data.c
@@ -4,7 +4,9 @@ 
 
 #include <string.h>
 #include <sys/socket.h>
+#ifndef RTE_EXEC_ENV_WINDOWS
 #include <sys/un.h>
+#endif
 #include <unistd.h>
 #include <limits.h>