Re: [PATCH v4 1/3] Cygwin: rewrite and make public cmdline parser

[Corinna Vinschen <corinna-cygwin@cygwin.com>]

On Sep  5 13:27, Mingye Wang wrote:
> This commit rewrites the cmdline parser to achieve the following:
> * MSVCRT compatibility. Except for the single-quote handling (an
>   extension for compatibility with old Cygwin), the parser now
>   interprets option boundaries exactly like MSVCR since 2008. This fixes
>   the issue where our escaping does not work with our own parsing.
> * Clarity. Since globify() is no longer responsible for handling the
>   opening and closing of quotes, the code is much simpler.
> * Sanity. The GLOB_NOCHECK flag is removed, so a failed glob correctly
>   returns the literal value. Without the change, anything path-like
>   would be garbled by globify's escaping.
> * A memory leak in the @file expansion is removed by rewriting it to use
>   a stack of buffers. This also simplifies the code since we no longer
>   have to move stuff. The "downside" is that tokens can no longer cross
>   file boundaries.
> 
> Some clarifications are made in the documentation for when globs are not
> expanded.  The functions are made public for testing, but my tcl setup
> is currently too messed up for running them!  I did test them as an
> isolated program on WSL-Debian.
> 
> The change fixes two complaints of mine:
> * That cygwin is incompatible with its own escape.[1]
> * That there is no way to echo `C:\"` from win32.[2]
>   [1]: https://cygwin.com/pipermail/cygwin/2020-June/245162.html
>   [2]: https://cygwin.com/pipermail/cygwin/2019-October/242790.html
> 
> (It's never the point to spawn cygwin32 from cygwin64. Consistency
> matters: with yourself always, and with the outside world when you are
> supposed to.)
> 
> This is the fourth version of the patch. I provide all my patches to
> Cygwin, including this one and all future ones, under the 2-clause BSD
> license.

Great, thanks.  A couple of (mostly minor) nits, though.

> diff --git a/winsup/cygwin/common.din b/winsup/cygwin/common.din
> index a7b4aa2b0..f15dc6a9e 100644
> --- a/winsup/cygwin/common.din
> +++ b/winsup/cygwin/common.din
> @@ -393,6 +393,8 @@ cygwin_split_path NOSIGFE
>  cygwin_stackdump SIGFE
>  cygwin_umount SIGFE
>  cygwin_winpid_to_pid SIGFE
> +cygwin_cmdline_parse SIGFE
> +cygwin_cmdline_build SIGFE

Nope, we won't do that.  The command line parsing is an internal
thing, and we won't export arbitrary internal functions using
their own symbol.  *If* we should export this stuff at all, which
I highly doubt as necessary, it should use the cygwin_internal API.

> diff --git a/winsup/cygwin/include/sys/cygwin.h b/winsup/cygwin/include/sys/cygwin.h
> index 805671ef9..e19ac0cd2 100644
> --- a/winsup/cygwin/include/sys/cygwin.h
> +++ b/winsup/cygwin/include/sys/cygwin.h
> @@ -86,6 +86,8 @@ extern void *cygwin_create_path (cygwin_conv_path_t what, const void *from);
>  extern pid_t cygwin_winpid_to_pid (int);
>  extern int cygwin_posix_path_list_p (const char *);
>  extern void cygwin_split_path (const char *, char *, char *);
> +extern int cygwin_cmdline_parse (char *, char ***, char **, int, int);
> +extern char *cygwin_cmdline_build (const char * const *, int, int);

Ditto.

> +static char* __reg1
> +read_file (const char *name)
> +{
> +#ifndef WINF_STDIO_TEST

Please drop this, together with the else branch for WSL.

> +      // For anything else, sort out backslashes first.
> +      // All backslashes are literal, except these before a quote.
> +      // Single-quote is our addition.  Would love to remove it.

Pleae use /* */ coments for multiline comments.

> +/* Perform a glob on word if it contains wildcard characters.
> +   Also quote every character between quotes to force glob to
> +   treat the characters literally.
> +
> +   Call glob(3) on the word, and fill argv accordingly.
> +   If the input looks like a DOS path, double up backslashes.
> + */

Please join the last two lines.  The closing */ should be on the
last comment line.

> +extern "C" int
> +extern "C" char *
> +extern "C"
> +{
> +  int cygwin_cmdline_parse (char *, char ***, char **, int, int);
> +  char *cygwin_cmdline_build (const char * const *, int, int);
> +}

Bzz.

> --- a/winsup/doc/misc-funcs.xml
> +++ b/winsup/doc/misc-funcs.xml

Ditto.

> diff --git a/winsup/testsuite/Makefile.in b/winsup/testsuite/Makefile.in
> index a86a35b88..bdc116d12 100644
> --- a/winsup/testsuite/Makefile.in
> +++ b/winsup/testsuite/Makefile.in

Skip it.  Just add this as a standalone testcase as attachement, that's
sufficient.


Thanks,
Corinna