[PATCH v4 1/3] Cygwin: rewrite and make public cmdline parser
Corinna Vinschen
corinna-cygwin@cygwin.com
Mon Sep 7 09:39:43 GMT 2020
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
More information about the Cygwin-patches
mailing list