[6/7] cmdline: support Windows
diff mbox series

Message ID 20200620210511.13134-7-dmitry.kozliuk@gmail.com
State Deferred
Delegated to: Thomas Monjalon
Headers show
Series
  • cmdline: support Windows
Related show

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/checkpatch success coding style OK

Commit Message

Dmitry Kozlyuk June 20, 2020, 9:05 p.m. UTC
Implement terminal handling, input polling, and vdprintf() for Windows.

Because Windows I/O model differs fundamentally from Unix and there is
no concept of character device, polling is simulated depending on the
underlying inpue device. Supporting non-terminal input is useful for
automated testing.

Windows emulation of VT100 uses "ESC [ E" for newline instead of
standard "ESC E", so a workaround is added.

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 config/meson.build                      |   2 +
 lib/librte_cmdline/cmdline.c            |   5 +
 lib/librte_cmdline/cmdline_os_windows.c | 207 ++++++++++++++++++++++++
 lib/librte_cmdline/cmdline_private.h    |  15 ++
 lib/librte_cmdline/cmdline_socket.c     |   4 +
 lib/librte_cmdline/cmdline_vt100.h      |   4 +
 lib/librte_cmdline/meson.build          |   4 +-
 lib/meson.build                         |   1 +
 8 files changed, 241 insertions(+), 1 deletion(-)
 create mode 100644 lib/librte_cmdline/cmdline_os_windows.c

Comments

Fady Bader June 28, 2020, 2:20 p.m. UTC | #1
Hi Dmitry,
I'm trying to run test-pmd on Windows and I ran into this error with cmdline.

The error log message is :
In file included from ../app/test-pmd/cmdline_flow.c:23:
..\lib\librte_cmdline/cmdline_parse_num.h:24:2: error: 'INT64' redeclared as different kind of symbol
  INT64

In file included from C:/mingw-w64/x86_64/mingw64/x86_64-w64-mingw32/include/winnt.h:150,
                 from C:/mingw-w64/x86_64/mingw64/x86_64-w64-mingw32/include/minwindef.h:163,
                 from C:/mingw-w64/x86_64/mingw64/x86_64-w64-mingw32/include/windef.h:8,
                 from C:/mingw-w64/x86_64/mingw64/x86_64-w64-mingw32/include/windows.h:69,
                 from ..\lib/librte_eal/windows/include/rte_windows.h:22,
                 from ..\lib/librte_eal/windows/include/pthread.h:20,
                 from ..\lib/librte_eal/include/rte_per_lcore.h:25,
                 from ..\lib/librte_eal/include/rte_errno.h:18,
                 from ..\lib\librte_ethdev/rte_ethdev.h:156,
                 from ../app/test-pmd/cmdline_flow.c:18:
C:/mingw-w64/x86_64/mingw64/x86_64-w64-mingw32/include/basetsd.h:32:44: note: previous declaration of 'INT64' was here
   __MINGW_EXTENSION typedef signed __int64 INT64,*PINT64;

The same error is for the other types defined in cmdline_numtype.

This problem with windows.h is popping in many places and some of them are 
cmdline and test-pmd and librte_net. 
We should find a way to exclude windows.h from the unneeded places, is there any
suggestions on how it can be done ?

> -----Original Message-----
> From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> Sent: Sunday, June 21, 2020 12:05 AM
> To: dev@dpdk.org
> Cc: Dmitry Malloy <dmitrym@microsoft.com>; Narcisa Ana Maria Vasile
> <Narcisa.Vasile@microsoft.com>; Fady Bader <fady@mellanox.com>; Tal
> Shnaiderman <talshn@mellanox.com>; Dmitry Kozlyuk
> <dmitry.kozliuk@gmail.com>; Thomas Monjalon <thomas@monjalon.net>;
> Olivier Matz <olivier.matz@6wind.com>
> Subject: [PATCH 6/7] cmdline: support Windows
> 
> Implement terminal handling, input polling, and vdprintf() for Windows.
> 
> Because Windows I/O model differs fundamentally from Unix and there is no
> concept of character device, polling is simulated depending on the underlying
> inpue device. Supporting non-terminal input is useful for automated testing.
> 
> Windows emulation of VT100 uses "ESC [ E" for newline instead of standard "ESC
> E", so a workaround is added.
> 
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
>  config/meson.build                      |   2 +
>  lib/librte_cmdline/cmdline.c            |   5 +
>  lib/librte_cmdline/cmdline_os_windows.c | 207 ++++++++++++++++++++++++
>  lib/librte_cmdline/cmdline_private.h    |  15 ++
>  lib/librte_cmdline/cmdline_socket.c     |   4 +
>  lib/librte_cmdline/cmdline_vt100.h      |   4 +
>  lib/librte_cmdline/meson.build          |   4 +-
>  lib/meson.build                         |   1 +
>  8 files changed, 241 insertions(+), 1 deletion(-)  create mode 100644
> lib/librte_cmdline/cmdline_os_windows.c
> 
> diff --git a/config/meson.build b/config/meson.build index d3f05f878..733b5e310
> 100644
> --- a/config/meson.build
> +++ b/config/meson.build
> @@ -269,6 +269,8 @@ if is_windows
>  		add_project_arguments('-D__USE_MINGW_ANSI_STDIO',
> language: 'c')
>  	endif
> 
> +	add_project_link_arguments('-lws2_32', language: 'c')
> +
>  	# Contrary to docs, VirtualAlloc2() is exported by mincore.lib
>  	# in Windows SDK, while MinGW exports it by advapi32.a.
>  	if is_ms_linker
> diff --git a/lib/librte_cmdline/cmdline.c b/lib/librte_cmdline/cmdline.c index
> 00b9e6b2e..c0ddb5f23 100644
> --- a/lib/librte_cmdline/cmdline.c
> +++ b/lib/librte_cmdline/cmdline.c
> @@ -13,9 +13,14 @@
>  #include <fcntl.h>
>  #include <errno.h>
>  #include <netinet/in.h>
> +#include <unistd.h>
> 
>  #include <rte_string_fns.h>
> 
> +#ifdef RTE_EXEC_ENV_WINDOWS
> +#define write _write
> +#endif
> +
>  #include "cmdline.h"
>  #include "cmdline_private.h"
> 
> diff --git a/lib/librte_cmdline/cmdline_os_windows.c
> b/lib/librte_cmdline/cmdline_os_windows.c
> new file mode 100644
> index 000000000..9736f6531
> --- /dev/null
> +++ b/lib/librte_cmdline/cmdline_os_windows.c
> @@ -0,0 +1,207 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright (c) 2020 Dmitry Kozlyuk
> + */
> +
> +#include <io.h>
> +
> +#include <rte_os.h>
> +
> +#include "cmdline_private.h"
> +
> +/* Missing from some MinGW-w64 distributions. */ #ifndef
> +ENABLE_VIRTUAL_TERMINAL_PROCESSING
> +#define ENABLE_VIRTUAL_TERMINAL_PROCESSING 0x0004 #endif
> +
> +#ifndef ENABLE_VIRTUAL_TERMINAL_INPUT
> +#define ENABLE_VIRTUAL_TERMINAL_INPUT 0x0200 #endif
> +
> +void
> +terminal_adjust(struct terminal *oldterm) {
> +	HANDLE handle;
> +	DWORD mode;
> +
> +	ZeroMemory(oldterm, sizeof(*oldterm));
> +
> +	/* Detect console input, set it up and make it emulate VT100. */
> +	handle = GetStdHandle(STD_INPUT_HANDLE);
> +	if (GetConsoleMode(handle, &mode)) {
> +		oldterm->is_console_input = 1;
> +		oldterm->input_mode = mode;
> +
> +		mode &= ~(
> +			ENABLE_LINE_INPUT |      /* no line buffering */
> +			ENABLE_ECHO_INPUT |      /* no echo */
> +			ENABLE_PROCESSED_INPUT | /* pass Ctrl+C to program
> */
> +			ENABLE_MOUSE_INPUT |     /* no mouse events */
> +			ENABLE_WINDOW_INPUT);    /* no window resize events
> */
> +		mode |= ENABLE_VIRTUAL_TERMINAL_INPUT;
> +		SetConsoleMode(handle, mode);
> +	}
> +
> +	/* Detect console output and make it emulate VT100. */
> +	handle = GetStdHandle(STD_OUTPUT_HANDLE);
> +	if (GetConsoleMode(handle, &mode)) {
> +		oldterm->is_console_output = 1;
> +		oldterm->output_mode = mode;
> +
> +		mode &= ~ENABLE_WRAP_AT_EOL_OUTPUT;
> +		mode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
> +		SetConsoleMode(handle, mode);
> +	}
> +}
> +
> +void
> +terminal_restore(const struct terminal *oldterm) {
> +	if (oldterm->is_console_input) {
> +		HANDLE handle = GetStdHandle(STD_INPUT_HANDLE);
> +		SetConsoleMode(handle, oldterm->input_mode);
> +	}
> +
> +	if (oldterm->is_console_output) {
> +		HANDLE handle = GetStdHandle(STD_OUTPUT_HANDLE);
> +		SetConsoleMode(handle, oldterm->output_mode);
> +	}
> +}
> +
> +static int
> +cmdline_is_key_down(const INPUT_RECORD *record) {
> +	return (record->EventType == KEY_EVENT) &&
> +		record->Event.KeyEvent.bKeyDown;
> +}
> +
> +static int
> +cmdline_poll_char_console(HANDLE handle) {
> +	INPUT_RECORD record;
> +	DWORD events;
> +
> +	if (!PeekConsoleInput(handle, &record, 1, &events)) {
> +		/* Simulate poll(3) behavior on EOF. */
> +		return (GetLastError() == ERROR_HANDLE_EOF) ? 1 : -1;
> +	}
> +
> +	if ((events == 0) || !cmdline_is_key_down(&record))
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static int
> +cmdline_poll_char_file(struct cmdline *cl, HANDLE handle) {
> +	DWORD type = GetFileType(handle);
> +
> +	/* Since console is handled by cmdline_poll_char_console(),
> +	 * this is either a serial port or input handle had been replaced.
> +	 */
> +	if (type == FILE_TYPE_CHAR)
> +		return cmdline_poll_char_console(handle);
> +
> +	/* PeekNamedPipe() can handle all pipes and also sockets. */
> +	if (type == FILE_TYPE_PIPE) {
> +		DWORD bytes_avail;
> +		if (!PeekNamedPipe(handle, NULL, 0, NULL, &bytes_avail, NULL))
> +			return (GetLastError() == ERROR_BROKEN_PIPE) ? 1 : -1;
> +		return bytes_avail ? 1 : 0;
> +	}
> +
> +	/* There is no straightforward way to peek a file in Windows
> +	 * I/O model. Read the byte, if it is not the end of file,
> +	 * buffer it for subsequent read. This will not work with
> +	 * a file being appended and probably some other edge cases.
> +	 */
> +	if (type == FILE_TYPE_DISK) {
> +		char c;
> +		int ret;
> +
> +		ret = _read(cl->s_in, &c, sizeof(c));
> +		if (ret == 1) {
> +			cl->repeat_count = 1;
> +			cl->repeated_char = c;
> +		}
> +		return ret;
> +	}
> +
> +	/* GetFileType() failed or file of unknown type,
> +	 * which we do not know how to peek anyway.
> +	 */
> +	return -1;
> +}
> +
> +int
> +cmdline_poll_char(struct cmdline *cl)
> +{
> +	HANDLE handle = (HANDLE)_get_osfhandle(cl->s_in);
> +	return cl->oldterm.is_console_input ?
> +		cmdline_poll_char_console(handle) :
> +		cmdline_poll_char_file(cl, handle);
> +}
> +
> +ssize_t
> +cmdline_read_char(struct cmdline *cl, char *c) {
> +	HANDLE handle;
> +	INPUT_RECORD record;
> +	KEY_EVENT_RECORD *key;
> +	DWORD events;
> +
> +	if (!cl->oldterm.is_console_input)
> +		return _read(cl->s_in, c, 1);
> +
> +	/* Return repeated strokes from previous event. */
> +	if (cl->repeat_count > 0) {
> +		*c = cl->repeated_char;
> +		cl->repeat_count--;
> +		return 1;
> +	}
> +
> +	handle = (HANDLE)_get_osfhandle(cl->s_in);
> +	key = &record.Event.KeyEvent;
> +	do {
> +		if (!ReadConsoleInput(handle, &record, 1, &events)) {
> +			if (GetLastError() == ERROR_HANDLE_EOF) {
> +				*c = EOF;
> +				return 0;
> +			}
> +			return -1;
> +		}
> +	} while (!cmdline_is_key_down(&record));
> +
> +	*c = key->uChar.AsciiChar;
> +
> +	/* Save repeated strokes from a single event. */
> +	if (key->wRepeatCount > 1) {
> +		cl->repeated_char = *c;
> +		cl->repeat_count = key->wRepeatCount - 1;
> +	}
> +
> +	return 1;
> +}
> +
> +int
> +cmdline_vdprintf(int fd, const char *format, va_list op) {
> +	int copy, ret;
> +	FILE *file;
> +
> +	copy = _dup(fd);
> +	if (copy < 0)
> +		return -1;
> +
> +	file = _fdopen(copy, "a");
> +	if (file == NULL) {
> +		_close(copy);
> +		return -1;
> +	}
> +
> +	ret = vfprintf(file, format, op);
> +
> +	fclose(file); /* also closes copy */
> +
> +	return ret;
> +}
> diff --git a/lib/librte_cmdline/cmdline_private.h
> b/lib/librte_cmdline/cmdline_private.h
> index 338d3d55c..1e05ec376 100644
> --- a/lib/librte_cmdline/cmdline_private.h
> +++ b/lib/librte_cmdline/cmdline_private.h
> @@ -5,7 +5,11 @@
>  #ifndef _CMDLINE_PRIVATE_H_
>  #define _CMDLINE_PRIVATE_H_
> 
> +#ifdef RTE_EXEC_ENV_WINDOWS
> +#include <rte_windows.h>
> +#else
>  #include <termios.h>
> +#endif
> 
>  #include <stdarg.h>
> 
> @@ -15,7 +19,14 @@
>  #include <cmdline_parse.h>
> 
>  struct terminal {
> +#ifndef RTE_EXEC_ENV_WINDOWS
>  	struct termios termios;
> +#else
> +	DWORD input_mode;
> +	DWORD output_mode;
> +	int is_console_input;
> +	int is_console_output;
> +#endif
>  };
> 
>  /* Disable buffering and echoing, save previous settings to oldterm. */ @@ -31,6
> +42,10 @@ struct cmdline {
>  	struct rdline rdl;
>  	char prompt[RDLINE_PROMPT_SIZE];
>  	struct terminal oldterm;
> +#ifdef RTE_EXEC_ENV_WINDOWS
> +	char repeated_char;
> +	WORD repeat_count;
> +#endif
>  };
> 
>  /* Check if a single character can be read from input. */ diff --git
> a/lib/librte_cmdline/cmdline_socket.c b/lib/librte_cmdline/cmdline_socket.c
> index e73666f15..c5f483413 100644
> --- a/lib/librte_cmdline/cmdline_socket.c
> +++ b/lib/librte_cmdline/cmdline_socket.c
> @@ -16,6 +16,10 @@
>  #include "cmdline_private.h"
>  #include "cmdline_socket.h"
> 
> +#ifdef RTE_EXEC_ENV_WINDOWS
> +#define open _open
> +#endif
> +
>  struct cmdline *
>  cmdline_file_new(cmdline_parse_ctx_t *ctx, const char *prompt, const char
> *path)  { diff --git a/lib/librte_cmdline/cmdline_vt100.h
> b/lib/librte_cmdline/cmdline_vt100.h
> index e33e67ed8..be9ae8e1c 100644
> --- a/lib/librte_cmdline/cmdline_vt100.h
> +++ b/lib/librte_cmdline/cmdline_vt100.h
> @@ -31,7 +31,11 @@ extern "C" {
>  #define vt100_multi_right  "\033\133%uC"
>  #define vt100_multi_left   "\033\133%uD"
>  #define vt100_suppr        "\033\133\063\176"
> +#ifndef RTE_EXEC_ENV_WINDOWS
>  #define vt100_home         "\033M\033E"
> +#else
> +#define vt100_home         "\033M\033[E"
> +#endif
>  #define vt100_word_left    "\033\142"
>  #define vt100_word_right   "\033\146"
> 
> diff --git a/lib/librte_cmdline/meson.build b/lib/librte_cmdline/meson.build
> index 5c9e8886d..5009b3354 100644
> --- a/lib/librte_cmdline/meson.build
> +++ b/lib/librte_cmdline/meson.build
> @@ -25,7 +25,9 @@ headers = files('cmdline.h',
>  	'cmdline_cirbuf.h',
>  	'cmdline_parse_portlist.h')
> 
> -if not is_windows
> +if is_windows
> +	sources += files('cmdline_os_windows.c') else
>  	sources += files('cmdline_os_unix.c')
>  endif
> 
> diff --git a/lib/meson.build b/lib/meson.build index 40e452025..5b72a2d9e
> 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -40,6 +40,7 @@ if is_windows
>  		'kvargs','eal',
>  		'ring',
>  		'mempool', 'mbuf', 'pci', 'net',
> +		'cmdline',
>  	] # only supported libraries for windows  endif
> 
> --
> 2.25.4
Ranjit Menon June 29, 2020, 6:23 a.m. UTC | #2
On 6/28/2020 7:20 AM, Fady Bader wrote:
> Hi Dmitry,
> I'm trying to run test-pmd on Windows and I ran into this error with cmdline.
>
> The error log message is :
> In file included from ../app/test-pmd/cmdline_flow.c:23:
> ..\lib\librte_cmdline/cmdline_parse_num.h:24:2: error: 'INT64' redeclared as different kind of symbol
>    INT64
>
> In file included from C:/mingw-w64/x86_64/mingw64/x86_64-w64-mingw32/include/winnt.h:150,
>                   from C:/mingw-w64/x86_64/mingw64/x86_64-w64-mingw32/include/minwindef.h:163,
>                   from C:/mingw-w64/x86_64/mingw64/x86_64-w64-mingw32/include/windef.h:8,
>                   from C:/mingw-w64/x86_64/mingw64/x86_64-w64-mingw32/include/windows.h:69,
>                   from ..\lib/librte_eal/windows/include/rte_windows.h:22,
>                   from ..\lib/librte_eal/windows/include/pthread.h:20,
>                   from ..\lib/librte_eal/include/rte_per_lcore.h:25,
>                   from ..\lib/librte_eal/include/rte_errno.h:18,
>                   from ..\lib\librte_ethdev/rte_ethdev.h:156,
>                   from ../app/test-pmd/cmdline_flow.c:18:
> C:/mingw-w64/x86_64/mingw64/x86_64-w64-mingw32/include/basetsd.h:32:44: note: previous declaration of 'INT64' was here
>     __MINGW_EXTENSION typedef signed __int64 INT64,*PINT64;
>
> The same error is for the other types defined in cmdline_numtype.
>
> This problem with windows.h is popping in many places and some of them are
> cmdline and test-pmd and librte_net.
> We should find a way to exclude windows.h from the unneeded places, is there any
> suggestions on how it can be done ?

We ran into this same issue when working with the code that is on the 
draft repo.

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.

<snip>

ranjit m.
Dmitry Kozlyuk June 29, 2020, 7:42 a.m. UTC | #3
On Sun, 28 Jun 2020 23:23:11 -0700, Ranjit Menon wrote:
> On 6/28/2020 7:20 AM, Fady Bader wrote:
> > Hi Dmitry,
> > I'm trying to run test-pmd on Windows and I ran into this error with cmdline.
> >
> > The error log message is :
> > In file included from ../app/test-pmd/cmdline_flow.c:23:
> > ..\lib\librte_cmdline/cmdline_parse_num.h:24:2: error: 'INT64' redeclared as different kind of symbol
> >    INT64
> >
> > In file included from C:/mingw-w64/x86_64/mingw64/x86_64-w64-mingw32/include/winnt.h:150,
> >                   from C:/mingw-w64/x86_64/mingw64/x86_64-w64-mingw32/include/minwindef.h:163,
> >                   from C:/mingw-w64/x86_64/mingw64/x86_64-w64-mingw32/include/windef.h:8,
> >                   from C:/mingw-w64/x86_64/mingw64/x86_64-w64-mingw32/include/windows.h:69,
> >                   from ..\lib/librte_eal/windows/include/rte_windows.h:22,
> >                   from ..\lib/librte_eal/windows/include/pthread.h:20,
> >                   from ..\lib/librte_eal/include/rte_per_lcore.h:25,
> >                   from ..\lib/librte_eal/include/rte_errno.h:18,
> >                   from ..\lib\librte_ethdev/rte_ethdev.h:156,
> >                   from ../app/test-pmd/cmdline_flow.c:18:
> > C:/mingw-w64/x86_64/mingw64/x86_64-w64-mingw32/include/basetsd.h:32:44: note: previous declaration of 'INT64' was here
> >     __MINGW_EXTENSION typedef signed __int64 INT64,*PINT64;
> >
> > The same error is for the other types defined in cmdline_numtype.
> >
> > This problem with windows.h is popping in many places and some of them are
> > cmdline and test-pmd and librte_net.
> > We should find a way to exclude windows.h from the unneeded places, is there any
> > suggestions on how it can be done ?  
> 
> We ran into this same issue when working with the code that is on the 
> draft repo.
> 
> 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".

I don't see a clean way not to expose windows.h, because pthread.h depends on
it, and if we hide implementation, librte_eal would have to export pthread
symbols on Windows, which is a hack (or is it?).
Tal Shnaiderman June 29, 2020, 8:12 a.m. UTC | #4
> 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:
> > On 6/28/2020 7:20 AM, Fady Bader wrote:
> > > Hi Dmitry,
> > > I'm trying to run test-pmd on Windows and I ran into this error with
> cmdline.
> > >
> > > The error log message is :
> > > In file included from ../app/test-pmd/cmdline_flow.c:23:
> > > ..\lib\librte_cmdline/cmdline_parse_num.h:24:2: error: 'INT64'
> redeclared as different kind of symbol
> > >    INT64
> > >
> > > In file included from C:/mingw-w64/x86_64/mingw64/x86_64-w64-
> mingw32/include/winnt.h:150,
> > >                   from C:/mingw-w64/x86_64/mingw64/x86_64-w64-
> mingw32/include/minwindef.h:163,
> > >                   from C:/mingw-w64/x86_64/mingw64/x86_64-w64-
> mingw32/include/windef.h:8,
> > >                   from C:/mingw-w64/x86_64/mingw64/x86_64-w64-
> mingw32/include/windows.h:69,
> > >                   from ..\lib/librte_eal/windows/include/rte_windows.h:22,
> > >                   from ..\lib/librte_eal/windows/include/pthread.h:20,
> > >                   from ..\lib/librte_eal/include/rte_per_lcore.h:25,
> > >                   from ..\lib/librte_eal/include/rte_errno.h:18,
> > >                   from ..\lib\librte_ethdev/rte_ethdev.h:156,
> > >                   from ../app/test-pmd/cmdline_flow.c:18:
> > > C:/mingw-w64/x86_64/mingw64/x86_64-w64-
> mingw32/include/basetsd.h:32:44: note: previous declaration of 'INT64' was
> here
> > >     __MINGW_EXTENSION typedef signed __int64 INT64,*PINT64;
> > >
> > > The same error is for the other types defined in cmdline_numtype.
> > >
> > > This problem with windows.h is popping in many places and some of
> > > them are cmdline and test-pmd and librte_net.
> > > We should find a way to exclude windows.h from the unneeded places,
> > > is there any suggestions on how it can be done ?
> >
> > We ran into this same issue when working with the code that is on the
> > draft repo.
> >
> > 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".
> 
> I don't see a clean way not to expose windows.h, because pthread.h
> depends on it, and if we hide implementation, librte_eal would have to
> export pthread symbols on Windows, which is a hack (or is it?).

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.

> 
> --
> Dmitry Kozlyuk
Dmitry Kozlyuk June 29, 2020, 11:56 p.m. UTC | #5
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:  
> > > On 6/28/2020 7:20 AM, Fady Bader wrote:  
> > > > Hi Dmitry,
> > > > I'm trying to run test-pmd on Windows and I ran into this error with  
> > cmdline.  
> > > >
> > > > The error log message is :
> > > > In file included from ../app/test-pmd/cmdline_flow.c:23:
> > > > ..\lib\librte_cmdline/cmdline_parse_num.h:24:2: error: 'INT64'  
> > redeclared as different kind of symbol  
> > > >    INT64
> > > >
> > > > In file included from C:/mingw-w64/x86_64/mingw64/x86_64-w64-  
> > mingw32/include/winnt.h:150,  
> > > >                   from C:/mingw-w64/x86_64/mingw64/x86_64-w64-  
> > mingw32/include/minwindef.h:163,  
> > > >                   from C:/mingw-w64/x86_64/mingw64/x86_64-w64-  
> > mingw32/include/windef.h:8,  
> > > >                   from C:/mingw-w64/x86_64/mingw64/x86_64-w64-  
> > mingw32/include/windows.h:69,  
> > > >                   from ..\lib/librte_eal/windows/include/rte_windows.h:22,
> > > >                   from ..\lib/librte_eal/windows/include/pthread.h:20,
> > > >                   from ..\lib/librte_eal/include/rte_per_lcore.h:25,
> > > >                   from ..\lib/librte_eal/include/rte_errno.h:18,
> > > >                   from ..\lib\librte_ethdev/rte_ethdev.h:156,
> > > >                   from ../app/test-pmd/cmdline_flow.c:18:
> > > > C:/mingw-w64/x86_64/mingw64/x86_64-w64-  
> > mingw32/include/basetsd.h:32:44: note: previous declaration of 'INT64' was
> > here  
> > > >     __MINGW_EXTENSION typedef signed __int64 INT64,*PINT64;
> > > >
> > > > The same error is for the other types defined in cmdline_numtype.
> > > >
> > > > This problem with windows.h is popping in many places and some of
> > > > them are cmdline and test-pmd and librte_net.
> > > > We should find a way to exclude windows.h from the unneeded places,
> > > > is there any suggestions on how it can be done ?  
> > >
> > > We ran into this same issue when working with the code that is on the
> > > draft repo.
> > >
> > > 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".
> > 
> > I don't see a clean way not to expose windows.h, because pthread.h
> > depends on it, and if we hide implementation, librte_eal would have to
> > export pthread symbols on Windows, which is a hack (or is it?).  
> 
> 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.


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:

CloseHandle
CreateThread
DeleteSynchronizationBarrier
EnterSynchronizationBarrier
GetThreadAffinityMask
InitializeSynchronizationBarrier
OpenThread
SetPriorityClass
SetThreadAffinityMask
SetThreadPriority
TerminateThread

Windows has strict compatibility policy, so prototypes are unlikely to ever
change. None of the used functions takes string parameters, thus not affected
by A/W macros. Looks a bit messy, but it's limited in scope at least.


An external pthread library would solve the problem, but as I've reported
earlier, I failed to find a good one: [1] and [3] are tied to MinGW, although
of high quality, [2] seems outdated.

[1]: Wnpthreads:
https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-libraries/winpthreads/
[2] pthreads-win32: https://sourceware.org/pthreads-win32/
[3] mcfgthread: https://github.com/lhmouse/mcfgthread
Dmitry Kozlyuk July 8, 2020, 1:09 a.m. UTC | #6
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.

Patch
diff mbox series

diff --git a/config/meson.build b/config/meson.build
index d3f05f878..733b5e310 100644
--- a/config/meson.build
+++ b/config/meson.build
@@ -269,6 +269,8 @@  if is_windows
 		add_project_arguments('-D__USE_MINGW_ANSI_STDIO', language: 'c')
 	endif
 
+	add_project_link_arguments('-lws2_32', language: 'c')
+
 	# Contrary to docs, VirtualAlloc2() is exported by mincore.lib
 	# in Windows SDK, while MinGW exports it by advapi32.a.
 	if is_ms_linker
diff --git a/lib/librte_cmdline/cmdline.c b/lib/librte_cmdline/cmdline.c
index 00b9e6b2e..c0ddb5f23 100644
--- a/lib/librte_cmdline/cmdline.c
+++ b/lib/librte_cmdline/cmdline.c
@@ -13,9 +13,14 @@ 
 #include <fcntl.h>
 #include <errno.h>
 #include <netinet/in.h>
+#include <unistd.h>
 
 #include <rte_string_fns.h>
 
+#ifdef RTE_EXEC_ENV_WINDOWS
+#define write _write
+#endif
+
 #include "cmdline.h"
 #include "cmdline_private.h"
 
diff --git a/lib/librte_cmdline/cmdline_os_windows.c b/lib/librte_cmdline/cmdline_os_windows.c
new file mode 100644
index 000000000..9736f6531
--- /dev/null
+++ b/lib/librte_cmdline/cmdline_os_windows.c
@@ -0,0 +1,207 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2020 Dmitry Kozlyuk
+ */
+
+#include <io.h>
+
+#include <rte_os.h>
+
+#include "cmdline_private.h"
+
+/* Missing from some MinGW-w64 distributions. */
+#ifndef ENABLE_VIRTUAL_TERMINAL_PROCESSING
+#define ENABLE_VIRTUAL_TERMINAL_PROCESSING 0x0004
+#endif
+
+#ifndef ENABLE_VIRTUAL_TERMINAL_INPUT
+#define ENABLE_VIRTUAL_TERMINAL_INPUT 0x0200
+#endif
+
+void
+terminal_adjust(struct terminal *oldterm)
+{
+	HANDLE handle;
+	DWORD mode;
+
+	ZeroMemory(oldterm, sizeof(*oldterm));
+
+	/* Detect console input, set it up and make it emulate VT100. */
+	handle = GetStdHandle(STD_INPUT_HANDLE);
+	if (GetConsoleMode(handle, &mode)) {
+		oldterm->is_console_input = 1;
+		oldterm->input_mode = mode;
+
+		mode &= ~(
+			ENABLE_LINE_INPUT |      /* no line buffering */
+			ENABLE_ECHO_INPUT |      /* no echo */
+			ENABLE_PROCESSED_INPUT | /* pass Ctrl+C to program */
+			ENABLE_MOUSE_INPUT |     /* no mouse events */
+			ENABLE_WINDOW_INPUT);    /* no window resize events */
+		mode |= ENABLE_VIRTUAL_TERMINAL_INPUT;
+		SetConsoleMode(handle, mode);
+	}
+
+	/* Detect console output and make it emulate VT100. */
+	handle = GetStdHandle(STD_OUTPUT_HANDLE);
+	if (GetConsoleMode(handle, &mode)) {
+		oldterm->is_console_output = 1;
+		oldterm->output_mode = mode;
+
+		mode &= ~ENABLE_WRAP_AT_EOL_OUTPUT;
+		mode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
+		SetConsoleMode(handle, mode);
+	}
+}
+
+void
+terminal_restore(const struct terminal *oldterm)
+{
+	if (oldterm->is_console_input) {
+		HANDLE handle = GetStdHandle(STD_INPUT_HANDLE);
+		SetConsoleMode(handle, oldterm->input_mode);
+	}
+
+	if (oldterm->is_console_output) {
+		HANDLE handle = GetStdHandle(STD_OUTPUT_HANDLE);
+		SetConsoleMode(handle, oldterm->output_mode);
+	}
+}
+
+static int
+cmdline_is_key_down(const INPUT_RECORD *record)
+{
+	return (record->EventType == KEY_EVENT) &&
+		record->Event.KeyEvent.bKeyDown;
+}
+
+static int
+cmdline_poll_char_console(HANDLE handle)
+{
+	INPUT_RECORD record;
+	DWORD events;
+
+	if (!PeekConsoleInput(handle, &record, 1, &events)) {
+		/* Simulate poll(3) behavior on EOF. */
+		return (GetLastError() == ERROR_HANDLE_EOF) ? 1 : -1;
+	}
+
+	if ((events == 0) || !cmdline_is_key_down(&record))
+		return 0;
+
+	return 1;
+}
+
+static int
+cmdline_poll_char_file(struct cmdline *cl, HANDLE handle)
+{
+	DWORD type = GetFileType(handle);
+
+	/* Since console is handled by cmdline_poll_char_console(),
+	 * this is either a serial port or input handle had been replaced.
+	 */
+	if (type == FILE_TYPE_CHAR)
+		return cmdline_poll_char_console(handle);
+
+	/* PeekNamedPipe() can handle all pipes and also sockets. */
+	if (type == FILE_TYPE_PIPE) {
+		DWORD bytes_avail;
+		if (!PeekNamedPipe(handle, NULL, 0, NULL, &bytes_avail, NULL))
+			return (GetLastError() == ERROR_BROKEN_PIPE) ? 1 : -1;
+		return bytes_avail ? 1 : 0;
+	}
+
+	/* There is no straightforward way to peek a file in Windows
+	 * I/O model. Read the byte, if it is not the end of file,
+	 * buffer it for subsequent read. This will not work with
+	 * a file being appended and probably some other edge cases.
+	 */
+	if (type == FILE_TYPE_DISK) {
+		char c;
+		int ret;
+
+		ret = _read(cl->s_in, &c, sizeof(c));
+		if (ret == 1) {
+			cl->repeat_count = 1;
+			cl->repeated_char = c;
+		}
+		return ret;
+	}
+
+	/* GetFileType() failed or file of unknown type,
+	 * which we do not know how to peek anyway.
+	 */
+	return -1;
+}
+
+int
+cmdline_poll_char(struct cmdline *cl)
+{
+	HANDLE handle = (HANDLE)_get_osfhandle(cl->s_in);
+	return cl->oldterm.is_console_input ?
+		cmdline_poll_char_console(handle) :
+		cmdline_poll_char_file(cl, handle);
+}
+
+ssize_t
+cmdline_read_char(struct cmdline *cl, char *c)
+{
+	HANDLE handle;
+	INPUT_RECORD record;
+	KEY_EVENT_RECORD *key;
+	DWORD events;
+
+	if (!cl->oldterm.is_console_input)
+		return _read(cl->s_in, c, 1);
+
+	/* Return repeated strokes from previous event. */
+	if (cl->repeat_count > 0) {
+		*c = cl->repeated_char;
+		cl->repeat_count--;
+		return 1;
+	}
+
+	handle = (HANDLE)_get_osfhandle(cl->s_in);
+	key = &record.Event.KeyEvent;
+	do {
+		if (!ReadConsoleInput(handle, &record, 1, &events)) {
+			if (GetLastError() == ERROR_HANDLE_EOF) {
+				*c = EOF;
+				return 0;
+			}
+			return -1;
+		}
+	} while (!cmdline_is_key_down(&record));
+
+	*c = key->uChar.AsciiChar;
+
+	/* Save repeated strokes from a single event. */
+	if (key->wRepeatCount > 1) {
+		cl->repeated_char = *c;
+		cl->repeat_count = key->wRepeatCount - 1;
+	}
+
+	return 1;
+}
+
+int
+cmdline_vdprintf(int fd, const char *format, va_list op)
+{
+	int copy, ret;
+	FILE *file;
+
+	copy = _dup(fd);
+	if (copy < 0)
+		return -1;
+
+	file = _fdopen(copy, "a");
+	if (file == NULL) {
+		_close(copy);
+		return -1;
+	}
+
+	ret = vfprintf(file, format, op);
+
+	fclose(file); /* also closes copy */
+
+	return ret;
+}
diff --git a/lib/librte_cmdline/cmdline_private.h b/lib/librte_cmdline/cmdline_private.h
index 338d3d55c..1e05ec376 100644
--- a/lib/librte_cmdline/cmdline_private.h
+++ b/lib/librte_cmdline/cmdline_private.h
@@ -5,7 +5,11 @@ 
 #ifndef _CMDLINE_PRIVATE_H_
 #define _CMDLINE_PRIVATE_H_
 
+#ifdef RTE_EXEC_ENV_WINDOWS
+#include <rte_windows.h>
+#else
 #include <termios.h>
+#endif
 
 #include <stdarg.h>
 
@@ -15,7 +19,14 @@ 
 #include <cmdline_parse.h>
 
 struct terminal {
+#ifndef RTE_EXEC_ENV_WINDOWS
 	struct termios termios;
+#else
+	DWORD input_mode;
+	DWORD output_mode;
+	int is_console_input;
+	int is_console_output;
+#endif
 };
 
 /* Disable buffering and echoing, save previous settings to oldterm. */
@@ -31,6 +42,10 @@  struct cmdline {
 	struct rdline rdl;
 	char prompt[RDLINE_PROMPT_SIZE];
 	struct terminal oldterm;
+#ifdef RTE_EXEC_ENV_WINDOWS
+	char repeated_char;
+	WORD repeat_count;
+#endif
 };
 
 /* Check if a single character can be read from input. */
diff --git a/lib/librte_cmdline/cmdline_socket.c b/lib/librte_cmdline/cmdline_socket.c
index e73666f15..c5f483413 100644
--- a/lib/librte_cmdline/cmdline_socket.c
+++ b/lib/librte_cmdline/cmdline_socket.c
@@ -16,6 +16,10 @@ 
 #include "cmdline_private.h"
 #include "cmdline_socket.h"
 
+#ifdef RTE_EXEC_ENV_WINDOWS
+#define open _open
+#endif
+
 struct cmdline *
 cmdline_file_new(cmdline_parse_ctx_t *ctx, const char *prompt, const char *path)
 {
diff --git a/lib/librte_cmdline/cmdline_vt100.h b/lib/librte_cmdline/cmdline_vt100.h
index e33e67ed8..be9ae8e1c 100644
--- a/lib/librte_cmdline/cmdline_vt100.h
+++ b/lib/librte_cmdline/cmdline_vt100.h
@@ -31,7 +31,11 @@  extern "C" {
 #define vt100_multi_right  "\033\133%uC"
 #define vt100_multi_left   "\033\133%uD"
 #define vt100_suppr        "\033\133\063\176"
+#ifndef RTE_EXEC_ENV_WINDOWS
 #define vt100_home         "\033M\033E"
+#else
+#define vt100_home         "\033M\033[E"
+#endif
 #define vt100_word_left    "\033\142"
 #define vt100_word_right   "\033\146"
 
diff --git a/lib/librte_cmdline/meson.build b/lib/librte_cmdline/meson.build
index 5c9e8886d..5009b3354 100644
--- a/lib/librte_cmdline/meson.build
+++ b/lib/librte_cmdline/meson.build
@@ -25,7 +25,9 @@  headers = files('cmdline.h',
 	'cmdline_cirbuf.h',
 	'cmdline_parse_portlist.h')
 
-if not is_windows
+if is_windows
+	sources += files('cmdline_os_windows.c')
+else
 	sources += files('cmdline_os_unix.c')
 endif
 
diff --git a/lib/meson.build b/lib/meson.build
index 40e452025..5b72a2d9e 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -40,6 +40,7 @@  if is_windows
 		'kvargs','eal',
 		'ring',
 		'mempool', 'mbuf', 'pci', 'net',
+		'cmdline',
 	] # only supported libraries for windows
 endif